Skip to content
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

Merged
merged 3 commits into from
Oct 13, 2023

Conversation

wty-Bryant
Copy link
Contributor

@wty-Bryant wty-Bryant commented Oct 13, 2023

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 setting appID as a field of env config so that it be retrieved directly from env config struct

@wty-Bryant wty-Bryant requested a review from a team as a code owner October 13, 2023 16:57
@@ -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`)
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

@lucix-aws lucix-aws Oct 13, 2023

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.

@isaiahvita
Copy link
Contributor

@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

Copy link
Contributor

@isaiahvita isaiahvita left a 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

@wty-Bryant wty-Bryant merged commit 1cc8d04 into main Oct 13, 2023
14 of 19 checks passed
@wty-Bryant wty-Bryant deleted the feat-optimize-ua-appID branch October 13, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants