-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: Powertools User-Agent for Lambda execution environment #246
feat: Powertools User-Agent for Lambda execution environment #246
Conversation
…Metrics. New PowertoolsEnvironment
…to Major.Minor.Build Update tests.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #246 +/- ##
===========================================
+ Coverage 56.29% 60.29% +3.99%
===========================================
Files 41 42 +1
Lines 1796 1851 +55
===========================================
+ Hits 1011 1116 +105
+ Misses 785 735 -50
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
libraries/src/AWS.Lambda.Powertools.Common/Core/IPowertoolsEnvironment.cs
Outdated
Show resolved
Hide resolved
Waiting on confirmation from SDK team |
…ment from powertoolsconfigurations. Update tests
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.
The approach looks good to me. I just added a couple comments to confirm behavior.
libraries/src/AWS.Lambda.Powertools.Common/Core/SystemWrapper.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
/// <inheritdoc /> | ||
public void SetExecutionEnvironment<T>(T type) |
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 don't know the whole system so it might already being doing this but make sure this is only called once per power tools assembly. These assembly look up calls are can be costly and we don't want to it for every request.
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.
Thanks for the feedback @normj . The implementation prevents multiple instances of SystemWrapper to be created. The instances that create systemwrapper are singletons. The constructors are called once for the lambda lifetime.
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
/// <inheritdoc /> | ||
public void SetExecutionEnvironment<T>(T type) | ||
{ | ||
_systemWrapper.SetExecutionEnvironment(type); |
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.
This one calls SystemWrapper. SetExecutionEnvironment
and the system Wrapper I believe calls PowertoolsEnvironment, I believe there are too many layers of abstractions.
Issue number: #213
Summary
Update the execution environment so that the appropriate metadata can be attached to the user agent string during service calls via the AWS SDK for .NET
Changes
PTFeature/Name/Version
New method to set the execution environment in the
AWS.Lambda.Powertools.Common
projectEach Utility will have to Set the execution context
User experience
User-Agent header is updated and includes the Utilities used in the request to an AWS service using AWS SDK
Checklist
Please leave checklist items unchecked if they do not apply to your change.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.