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

feat: Record AWS Lambda coldstarts #2403

Conversation

serkan-ozal
Copy link
Contributor

Which problem is this PR solving?

Adds recording AWS Lambda invocation coldstarts as Lambda invocation span attributes.

Short description of the changes

Decides whether an AWS Lambda invocation is coldstart or not by taking care of the following cases:

  • Non-first invocations are warm, not coldstart
  • If sandbox environment is initialized with provisioned concurrency, every invocations are warm, not coldstart
  • If sandbox has been initialized proactively before the actual request, every invocations are warm, not coldstart

@serkan-ozal serkan-ozal force-pushed the improvement/instrumentation/aws-lambda/record-coldstart branch from f56c54d to c77981f Compare August 26, 2024 14:32
@serkan-ozal serkan-ozal changed the title Record AWS Lambda coldstarts feat: Record AWS Lambda coldstarts Aug 26, 2024
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.67%. Comparing base (dfb2dff) to head (8159ac7).
Report is 231 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2403      +/-   ##
==========================================
- Coverage   90.97%   90.67%   -0.30%     
==========================================
  Files         146      156      +10     
  Lines        7492     7628     +136     
  Branches     1502     1575      +73     
==========================================
+ Hits         6816     6917     +101     
- Misses        676      711      +35     
Files with missing lines Coverage Δ
...-instrumentation-aws-lambda/src/instrumentation.ts 95.21% <100.00%> (+1.53%) ⬆️

... and 75 files with indirect coverage changes

requestIsColdStart = false;
} else {
// Check whether it is proactive initialization or not:
// https://aaronstuyvenberg.com/posts/understanding-proactive-initialization

Choose a reason for hiding this comment

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

🤯

@pichlermarc pichlermarc added the pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. label Sep 4, 2024
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I think this can be merged. I left a minor suggestion, but if you don't want to do it that's fine. Just comment and we'll merge this when you're ready.

@serkan-ozal
Copy link
Contributor Author

serkan-ozal commented Sep 4, 2024

Looks reasonable to me. I think this can be merged. I left a minor suggestion, but if you don't want to do it that's fine. Just comment and we'll merge this when you're ready.

@dyladan Are you suggesting to update semconv package and update all semconv attributes in all places accordingly? If so, I think it should be done in a separate PR. I have used SEMATTRS_... properties, because as far as I see, it is the general approach used by other instrumentation plugins too.

@dyladan
Copy link
Member

dyladan commented Sep 4, 2024

Looks reasonable to me. I think this can be merged. I left a minor suggestion, but if you don't want to do it that's fine. Just comment and we'll merge this when you're ready.

@dyladan Are you suggesting to update semconv package and update all semconv attributes in all places accordingly? If so, I think it should be done in a separate PR. I have used SEMATTRS_... properties, because as far as I see, it is the general approach used by other instrumentation plugins too.

I had been thinking only the newly introduced attribute would use the new format. The old formats are still exported for backward compatibility. but leaving it for a followup is also fine.

@dyladan dyladan merged commit bc69fff into open-telemetry:main Sep 4, 2024
21 checks passed
@dyladan dyladan mentioned this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:instrumentation-aws-lambda pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants