Skip to content

Enable overriding environment detection #60

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

Merged
merged 1 commit into from
Jan 5, 2021
Merged

Enable overriding environment detection #60

merged 1 commit into from
Jan 5, 2021

Conversation

02strich
Copy link
Contributor

Sometimes it would be great to override the default selected environment (e.g. to force usage of the Lambda environment outside of Lambda), which this change enables.

The code is based on #26.

Issue #, if available:

Description of changes:
This adds a new configuration setting that allows overriding/bypassing the environment detection. In practice this, for example, can be useful when looking to use the library in a lambda-like setting but without the classical env variables.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Member

@jaredcnance jaredcnance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Minor comments.

@@ -32,6 +37,19 @@ async def resolve_environment() -> Environment:
log.debug("Environment resolved from cache.")
return EnvironmentCache.environment

print(repr(Config.environment))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove please or use the logger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

if Config.environment:
if Config.environment.lower() == "lambda":
EnvironmentCache.environment = lambda_environment
elif Config.environment.lower() == "ec2":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're calling .lower() multiple times which will result in unnecessary string allocation. Can we just do this once at the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed -- good call

Sometimes it would be great to override the default selected environment (e.g. to force usage of the Lambda environment outside of Lambda), which this change enables.

The code is based on #26.
@jaredcnance jaredcnance merged commit 5aa2d87 into awslabs:master Jan 5, 2021
@hussam789
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle unknown environment values

The code logs an info message when it fails to understand the environment
override, but continues execution without setting the environment. This could
lead to unexpected behavior. Consider either raising an exception or setting a
default environment in the else clause.

aws_embedded_metrics/environment/environment_detector.py [42-49]

-if lower_configured_enviroment == "lambda":
+if lower_configured_environment == "lambda":
     EnvironmentCache.environment = lambda_environment
-elif lower_configured_enviroment == "ec2":
+elif lower_configured_environment == "ec2":
     EnvironmentCache.environment = ec2_environment
-elif lower_configured_enviroment == "default":
+elif lower_configured_environment == "default":
     EnvironmentCache.environment = default_environment
 else:
-    log.info("Failed to understand environment override: %s", Config.environment)
+    log.warning("Failed to understand environment override: %s, using default environment", Config.environment)
+    EnvironmentCache.environment = default_environment
  • Apply this suggestion
Suggestion importance[1-10]: 6

__

Why: The suggestion improves error handling by logging a warning and assigning a default environment when an unknown value is encountered, enhancing robustness; however, it changes the existing fallback behavior, which warrants a moderate score.

Low
General
Fix variable name typo

There's a typo in the variable name 'lower_configured_enviroment' (missing 'n').
This could cause confusion for future developers and inconsistency in the
codebase.

aws_embedded_metrics/environment/environment_detector.py [40-41]

 if Config.environment:
-    lower_configured_enviroment = Config.environment.lower()
+    lower_configured_environment = Config.environment.lower()
  • Apply this suggestion
Suggestion importance[1-10]: 5

__

Why: The suggestion fixes a clear typo in the variable name, which improves readability and consistency; however, it only results in a minor improvement.

Low
  • More

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