-
Notifications
You must be signed in to change notification settings - Fork 3
feat: support not enabling aws default features #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,16 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| #[cfg(feature = "s3-no-defaults")] | ||
| use async_profiler_agent::reporter::s3::{S3Reporter, S3ReporterConfig}; | ||
| use async_profiler_agent::{ | ||
| metadata::AgentMetadata, | ||
| profiler::{ProfilerBuilder, ProfilerOptionsBuilder}, | ||
| reporter::{ | ||
| local::LocalReporter, | ||
| s3::{S3Reporter, S3ReporterConfig}, | ||
| }, | ||
| reporter::local::LocalReporter, | ||
| }; | ||
| use std::time::Duration; | ||
|
|
||
| #[cfg(feature = "s3-no-defaults")] | ||
| use aws_config::BehaviorVersion; | ||
| use clap::{ArgGroup, Parser}; | ||
|
|
||
|
|
@@ -29,20 +29,27 @@ pub fn set_up_tracing() { | |
| .init(); | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Default, Parser)] | ||
| struct S3BucketArgs { | ||
| #[arg(long, requires = "bucket_owner", requires = "profiling_group")] | ||
| bucket: Option<String>, | ||
| #[arg(long)] | ||
| bucket_owner: Option<String>, | ||
| #[arg(long)] | ||
| profiling_group: Option<String>, | ||
| } | ||
|
|
||
| /// Simple program to test the profiler agent | ||
| #[derive(Parser, Debug)] | ||
| #[derive(Debug, Parser)] | ||
| #[command(group( | ||
| ArgGroup::new("options") | ||
| .required(true) | ||
| .args(["local", "bucket"]), | ||
| ))] | ||
| struct Args { | ||
| #[arg(long)] | ||
| profiling_group: Option<String>, | ||
| #[arg(long)] | ||
| bucket_owner: Option<String>, | ||
| #[arg(long, requires = "bucket_owner", requires = "profiling_group")] | ||
| bucket: Option<String>, | ||
| #[cfg(feature = "s3-no-defaults")] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of these feature flags make the example very hard to follow. Can we move all s3 features into their own struct, |
||
| #[command(flatten)] | ||
| bucket_args: S3BucketArgs, | ||
| #[arg(long)] | ||
| local: Option<String>, | ||
| #[arg(long)] | ||
|
|
@@ -57,6 +64,18 @@ struct Args { | |
| native_mem: Option<String>, | ||
| } | ||
|
|
||
| impl Args { | ||
| #[cfg(feature = "s3-no-defaults")] | ||
| fn s3_bucket_args(&self) -> S3BucketArgs { | ||
| self.bucket_args.clone() | ||
| } | ||
|
|
||
| #[cfg(not(feature = "s3-no-defaults"))] | ||
| fn s3_bucket_args(&self) -> S3BucketArgs { | ||
| S3BucketArgs::default() | ||
| } | ||
| } | ||
|
|
||
| #[allow(unexpected_cfgs)] | ||
| pub fn main() -> anyhow::Result<()> { | ||
| let args = Args::parse(); | ||
|
|
@@ -82,23 +101,24 @@ async fn main_internal(args: Args) -> Result<(), anyhow::Error> { | |
|
|
||
| let profiler = ProfilerBuilder::default(); | ||
|
|
||
| let profiler = match ( | ||
| args.local, | ||
| args.bucket, | ||
| args.bucket_owner, | ||
| args.profiling_group, | ||
| ) { | ||
| (Some(local), _, _, _) => profiler | ||
| let profiler = match (&args.local, args.s3_bucket_args()) { | ||
| (Some(local), S3BucketArgs { .. }) => profiler | ||
| .with_reporter(LocalReporter::new(local)) | ||
| .with_custom_agent_metadata(AgentMetadata::Other), | ||
| (_, Some(bucket), Some(bucket_owner), Some(profiling_group)) => { | ||
| profiler.with_reporter(S3Reporter::new(S3ReporterConfig { | ||
| sdk_config: &aws_config::defaults(BehaviorVersion::latest()).load().await, | ||
| bucket_owner: bucket_owner, | ||
| bucket_name: bucket, | ||
| profiling_group_name: profiling_group, | ||
| })) | ||
| } | ||
| #[cfg(feature = "s3-no-defaults")] | ||
| ( | ||
| _, | ||
| S3BucketArgs { | ||
| bucket: Some(bucket_name), | ||
| bucket_owner: Some(bucket_owner), | ||
| profiling_group: Some(profiling_group_name), | ||
| }, | ||
| ) => profiler.with_reporter(S3Reporter::new(S3ReporterConfig { | ||
| sdk_config: &aws_config::defaults(BehaviorVersion::latest()).load().await, | ||
| bucket_owner, | ||
| bucket_name, | ||
| profiling_group_name, | ||
| })), | ||
| _ => unreachable!(), | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised to see that these passed out of box, but I guess we don't actually test any network calls? Looks like we validate metadata parsing only, and nothing to do with s3?
Probably having an integration test that validates these directly e2e using real auth would be nice, but separate scope. I can cut an issue (low priority).
Without that, can you confirm that you've manually validated that the existing functionality (without the new features) works properly? IE no regression for old APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, the tests don't have network access. I'm not in the mood of setting up an AWS account so I can do AWS-in-the-loop tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, we can maybe do #47 someday
From offline - thanks for confirming that you tested locally e2e and existin gfunctionality works.