-
Notifications
You must be signed in to change notification settings - Fork 318
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
Refresh tracer metadata on CRaC snapshot restore #3036
base: main
Are you sure you want to change the base?
Conversation
💚 CLA has been signed |
👋 @ArtyomGabeev Thanks a lot for your contribution! It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it. Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it. |
❕ Build Aborted
Expand to view the summary
Build stats
Steps errorsExpand to view the steps failures
|
@@ -143,6 +143,8 @@ private void completeMetaData(Context lambdaContext) { | |||
String region = arnSegments[3]; | |||
String accountId = arnSegments[4]; | |||
|
|||
String logStreamName = lambdaContext.getLogStreamName(); |
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.
unfortunately, after snapshot restore, this method returns null, so lambda instance has <uknown> node name instead of CloudWatch log stream name.
AWS team replied, that this environment variable won't be available, since it's impossible to change env variables after snapshot it taken.
We need to decide for a fallback strategy, how to generate lambda instance identifier.
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've got a reply from AWS that neither AWS_LAMBDA_LOG_STREAM_NAME, neither context.getLogStreamName gonna be populated for SnapStart enabled functions - aws/aws-lambda-java-libs#402 .
I do not know, if Kibana product have some special handling of AWS lambda APM data and if it relies on the format of configured_name from service.node field.
In initial proposal, I suggest to simulate and generate similar to log group node name, but right now I think we do not need to tie (if possible) instance name to log group name at all.
If we need just to distinguish different lambda instances, may be pair of lambda version + ephemeral id is enough, but this is a breaking change, if we are going to change it for all AWS Lambdas.
Another option I'm considering is to introduce a property, which will be a strategy for lambda instance naming.
By default, we should still try to use AWS_LAMBDA_LOG_STREAM_NAME env variable, but we may allow another one, which will use auto generated name.
Hi! We just realized that we haven't looked into this issue in a while. We're sorry! We're labeling this issue as |
+1 |
05e4839
to
9ce97f8
Compare
Hi @ArtyomGabeev , thank you for this contribution it's really interesting! I'll be looking at this, it may take a little while as I'm also on other things. First thing though, can you please sign the contributor agreement |
elastic-apm-agent-premain/src/main/java/co/elastic/apm/agent/premain/AgentMain.java
Show resolved
Hide resolved
I'm trying to sing contribution agreement, but do no receive any confirmation email. |
You seem to have 2 email addresses, one is signed up but the one associated with this PR isn't. I've tried to add the latter email address, did you get an email? |
Thank you, looks like CLA is signed off. |
Hi! We just realized that we haven't looked into this issue in a while. We're sorry! We're labeling this issue as |
+1 |
Hi! We just realized that we haven't looked into this issue in a while. We're sorry! We're labeling this issue as |
+1 |
Hi! We just realized that we haven't looked into this issue in a while. We're sorry! We're labeling this issue as |
still pending :+1 |
run docs-build |
Hi! We just realized that we haven't looked into this issue in a while. We're sorry! We're labeling this issue as |
Hi! This issue has been stale for a while and we're going to close it as part of our cleanup procedure. We appreciate your contribution and would like to apologize if we have not been able to review it, due to the current heavy load of the team. Feel free to re-open this issue if you think it should stay open. Thank you for your contribution! |
What does this PR do?
Checklist
Initial proposal for #2982