Optimize logic of retrieving user agent appID from env config#2314
Optimize logic of retrieving user agent appID from env config#2314wty-Bryant merged 3 commits intomainfrom
Conversation
| } | ||
|
|
||
| func (c EnvConfig) getAppID(context.Context) (string, bool, error) { | ||
| appID := os.Getenv(`AWS_SDK_UA_APP_ID`) |
There was a problem hiding this comment.
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.
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.
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.
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.
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 |
isaiahvita
left a comment
There was a problem hiding this comment.
LGTM, wait until the relevant CI passes though before merging
In a previous PR, a new
appIDfield is introduced into request's user agent header. In that old PR,appIDset via env var is retrieved separately after iterating resolver chain's all configs, this pr now optimize this step by settingappIDas a field of env config so that it be retrieved directly from env config struct