-
Notifications
You must be signed in to change notification settings - Fork 656
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
Optimize logic of retrieving user agent appID from env config #2314
Conversation
config/env_config.go
Outdated
@@ -335,6 +335,11 @@ func (c EnvConfig) getDefaultsMode(ctx context.Context) (aws.DefaultsMode, bool, | |||
return c.DefaultsMode, true, nil | |||
} | |||
|
|||
func (c EnvConfig) getAppID(context.Context) (string, bool, error) { | |||
appID := os.Getenv(`AWS_SDK_UA_APP_ID`) |
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.
while this works, i dont think this is the typical setup. see the other fields in env_config.go
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.
Just want to lazy set this field in case it is modified, but you are right, I will change that
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 dont think there is actually any reasonable time interval between when NewEnvConfig
is called and when getAppId
is called. so i dont think lazily evaluating it here makes a difference
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.
Yeah - loadEnvConfig
first reads everything out of the environment into the env config struct, and then the "resolvers" e.g. getAppID
just read out of the struct. getAppID
shouldn't be querying the environment directly. See other fields in env_config
as @isaiahvita said.
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.
Just want to lazy set this field in case it is modified,
i dont think there is actually any reasonable time interval
There's no chance of changing or interval to speak of - a process' environment is static.
@wty-Bryant also, while i already know the context of this PR. it will be good to clearly explain the motivation for it in the description |
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.
LGTM, wait until the relevant CI passes though before merging
In a previous PR, a new
appID
field is introduced into request's user agent header. In that old PR,appID
set via env var is retrieved separately after iterating resolver chain's all configs, this pr now optimize this step by settingappID
as a field of env config so that it be retrieved directly from env config struct