Add user agent #79
Add user agent #79
Conversation
| @@ -278,6 +281,7 @@ async fn main() -> Result<(), BoxError> { | |||
| tracing_subscriber::fmt() | |||
| .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) | |||
| .with_thread_ids(true) | |||
| .with_ansi(false) // Disable ANSI colors | |||
There was a problem hiding this comment.
with the ansi color, it's unreadable from pure text. I think it will be easier to read and parse.
| @@ -43,6 +46,11 @@ impl Config { | |||
| &self.concurrency | |||
| } | |||
|
|
|||
| /// Returns the name of the app that is using the transfer manager, if it was provided. | |||
| pub fn app_name(&self) -> Option<&AppName> { | |||
There was a problem hiding this comment.
fix: I don't think we want to expose app name. (1) we allow taking an S3 client (today anyway) that can already have app name configured, (2) I don't think this is what mountpoint will be looking for out of a UA header.
Is there something I'm missing here about why we are exposing this?
There was a problem hiding this comment.
From my reading of the sdk, the app name is just the string for the SDK to add in the user agent with some format check.
-
I think if we take the s3 client and want to respect the setting from transfer manager, we will be adding the user agent setting from transfer manager to the passed in client as the app name.
-
what do you think mountpoint will be looking for?
I assume they just want a string, and I thought the app name is the wrap of string for SDK to be provided in the user agent.
There was a problem hiding this comment.
From my reading of the sdk, the app name is just the string for the SDK to add in the user agent with some format check.
App name (appId) comes from the UA SEP. It's meant for end user applications (though I've yet to see someone use it 🤷♂️ in any SDK). Mountpoint could go through that but they are more like framework metadata.
I think if we take the s3 client and want to respect the setting from transfer manager, we will be adding the user agent setting from transfer manager to the passed in client as the app name.
The issue is the user could have set app name on the client. I think maybe we just leave this off for now and gather requirements from mountpoint to guide us. I think this is their user agent implementation though. From the looks of it we capture a lot of the md/* fields they are gathering I think so maybe they just need lib/mountpoint-s3-client#<version>.
There was a problem hiding this comment.
the framework metadata is supposed to be used as frameworks owned by the SDKs.
For frameworks owned by the SDKs, framework metadata MUST be included in the header following this format: "lib" / name ["#" version]. Additional metadata MAY be appended.
It seems to be an implementation detail for SDKs to me. And mountpoint is more like the application user, but yeah, they are more like a framework for the end user?
I guess we can chat with them and see what are they expecting?
There was a problem hiding this comment.
Yeah we run into this in other places. I think the SEP is worded poorly here. e.g. Amplify isn't owned by us but they need the same ability with the Kotlin SDK to tack on their UA info (which they utilize framework metadata IIRC).
| @@ -43,6 +46,12 @@ impl Config { | |||
| &self.concurrency | |||
| } | |||
|
|
|||
| #[doc(hidden)] | |||
There was a problem hiding this comment.
style: Put the attributes first with doc comments above it (same elswewhere)
e.g.
/// Returns the framework metadata setting when using transfer manager
#[doc(hidden)]
pub fn framework_metadata(&self) -> Option<&FrameworkMetadata> { ... }This goes for any attributes.
There was a problem hiding this comment.
I actually saw that in rust-sdk we put the attributes above the comments. https://github.com/awslabs/aws-sdk-rust/blob/main/sdk/aws-runtime/src/user_agent.rs#L172-L191
what kind of style rules we have? Are we put multiple attributes above comments?
Issue #, if available:
Description of changes:
ConfigLoader, we can decide what to do when user pass in their own client via config directly later.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.