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.

Sign up for GitHub

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
Merged

Optimize logic of retrieving user agent appID from env config #2314

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cdcb681
Optimize and Merge user agent appID resolving from env config
Oct 13, 2023
14bd3fc
Add and Merge changelog for prev commit
Oct 13, 2023
fcbf63c
Modify and Merge env config logic of retrieving UA appID
Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changelog/4f0e4753f9534938a41748143638981c.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id": "4f0e4753-f953-4938-a417-48143638981c",
"type": "feature",
"description": "Modify logic of retrieving user agent appID from env config",
"modules": [
"config"
]
}
5 changes: 5 additions & 0 deletions config/env_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

return appID, len(appID) > 0, nil
}

// GetRetryMaxAttempts returns the value of AWS_MAX_ATTEMPTS if was specified,
// and not 0.
func (c EnvConfig) GetRetryMaxAttempts(ctx context.Context) (int, bool, error) {
Expand Down
4 changes: 0 additions & 4 deletions config/resolve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,6 @@ func resolveAppID(ctx context.Context, cfg *aws.Config, configs configs) error {
return err
}

// if app ID is set in env var, it should precedence shared config value
if appID := os.Getenv(`AWS_SDK_UA_APP_ID`); len(appID) > 0 {
ID = appID
}
cfg.AppID = ID
return nil
}
Expand Down