-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add user agent #79
Add user agent #79
Changes from 6 commits
c4cc2c9
c916e88
53fe67a
28e6546
d2ac285
70ab365
9a61f4e
7f6070a
a3417dd
ce4a588
5d15540
460f4af
298f8af
7e3cbad
31a55a1
6e557c8
85d3f07
46e7a72
b46fd9a
9b824bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ | |
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
use aws_types::app_name::AppName; | ||
|
||
use crate::metrics::unit::ByteUnit; | ||
use crate::types::{ConcurrencySetting, PartSize}; | ||
use std::cmp; | ||
|
@@ -18,6 +20,7 @@ pub struct Config { | |
multipart_threshold: PartSize, | ||
target_part_size: PartSize, | ||
concurrency: ConcurrencySetting, | ||
app_name: Option<AppName>, | ||
client: aws_sdk_s3::client::Client, | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
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.
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 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.
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? 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. 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). |
||
self.app_name.as_ref() | ||
} | ||
|
||
/// The Amazon S3 client instance that will be used to send requests to S3. | ||
pub fn client(&self) -> &aws_sdk_s3::Client { | ||
&self.client | ||
|
@@ -55,6 +63,7 @@ pub struct Builder { | |
multipart_threshold_part_size: PartSize, | ||
target_part_size: PartSize, | ||
concurrency: ConcurrencySetting, | ||
app_name: Option<AppName>, | ||
client: Option<aws_sdk_s3::Client>, | ||
} | ||
|
||
|
@@ -122,8 +131,20 @@ impl Builder { | |
self | ||
} | ||
|
||
/// Sets the name of the app that is using the client. | ||
/// | ||
/// This _optional_ name is used to identify the application in the user agent that | ||
/// gets sent along with requests. | ||
pub fn app_name(mut self, app_name: Option<AppName>) -> Self { | ||
self.app_name = app_name; | ||
self | ||
} | ||
|
||
/// Set an explicit S3 client to use. | ||
pub fn client(mut self, client: aws_sdk_s3::Client) -> Self { | ||
// TODO - decide the approach here: | ||
// - Convert the client to build to modify it based on other configs for transfer manager | ||
// - Instead of taking the client, take sdk-config/s3-config/builder? | ||
self.client = Some(client); | ||
self | ||
} | ||
|
@@ -134,6 +155,7 @@ impl Builder { | |
multipart_threshold: self.multipart_threshold_part_size, | ||
target_part_size: self.target_part_size, | ||
concurrency: self.concurrency, | ||
app_name: self.app_name, | ||
client: self.client.expect("client set"), | ||
} | ||
} | ||
|
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.
question: why?
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.
with the ansi color, it's unreadable from pure text. I think it will be easier to read and parse.