-
Notifications
You must be signed in to change notification settings - Fork 302
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
[Instrumentation.AWSLambda] Incoming FaaS Span attributes: detect and set attribute for cold start #2037
[Instrumentation.AWSLambda] Incoming FaaS Span attributes: detect and set attribute for cold start #2037
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2037 +/- ##
==========================================
+ Coverage 73.91% 75.77% +1.85%
==========================================
Files 267 20 -247
Lines 9615 227 -9388
==========================================
- Hits 7107 172 -6935
+ Misses 2508 55 -2453
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Consider if any tests needs to be added/extended.
var functionTags = AWSLambdaUtils.GetFunctionTags(input, context); | ||
// No parallel invocation of the same lambda handler. | ||
bool faasColdStart = isColdStart; | ||
if (faasColdStart) |
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 if
can be removed, it's fine to unconditionally set it to false here. Then we also might not need the local variable if you change it after calculating functionTags
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.
Yes, we can do it. I removed the local variable and moved setting of the internal one after the calculation of functionTags.
Yes, this should be unit-testable, please add or extend one to verify the coldstart attribute going from true on the first call to false on subsequent ones. |
I added test for the cold start tag check. |
One more thing, please extend changelog. |
changelog has been updated. |
Co-authored-by: Christian Neumüller <[email protected]>
Fixes #
Design discussion issue #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes