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

read tracing info from system properties #8368

Merged
merged 3 commits into from
May 19, 2023
Merged

Conversation

jdoherty
Copy link
Contributor

Opened to address the following feature request:

#7579

@jdoherty jdoherty requested a review from a team April 26, 2023 09:53
@mateuszrzeszutek
Copy link
Member

Also pinging @open-telemetry/lambda-extension-approvers

Should we also update the spec while we're at it? Quoting @tylerbenson's comment from #7579 :

I think the main question I have is can this be included in the relevant spec somehow? When I updated that section earlier this year, I didn't understand or expect that the environment variable was updated for each request as that's not something normal applications do. I think it's worth adding or linking to some documentation about this environment variable's purpose and odd behavior. I haven't found any docs on it.

@jdoherty
Copy link
Contributor Author

jdoherty commented May 3, 2023

@tylerbenson @laurit could you take a look at this please? Alexander has approved this but is not a repo owner. Any thoughts are appreciated. cheers

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx!

do you know if there is any official docs on this system property from AWS?

the order of precedency (env var vs system prop) looks different from:

https://github.com/aws/aws-xray-sdk-java/blob/711e48cfd54400e728f99bec3bb3baf417276b46/aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/contexts/LambdaSegmentContext.java#L43-L46

also, do you know why aws/aws-xray-sdk-java#251 is not resolved?

@tylerbenson
Copy link
Member

(I would also love to see better documentation about this system property and corresponding environment variable linked somewhere, including the quirky lifecycle of the environment variable that led to this PR.)

@smirnoal
Copy link

smirnoal commented May 8, 2023

I will chase both the SDK issue closure and updating the public docs

@jdoherty
Copy link
Contributor Author

@trask @tylerbenson now we seem to have broad agreement on the docs over on open-telemetry/semantic-conventions#27 can we approve this one?

@smirnoal
Copy link

smirnoal commented Dec 5, 2023

I will chase both the SDK issue closure and updating the public docs

both achieved, docs were updated: https://docs.aws.amazon.com/lambda/latest/dg/configuration-envvars.html#configuration-envvars-runtime

@tylerbenson
Copy link
Member

@smirnoal Thanks for the update. I'd like to suggest adding a bit more to the xray tracing header section... specifically I think you should note that this doesn't behave like a normal environment variable because it changes for each function invocation. When I first encountered this it was surprising to me and some systems don't expect that. I assume Java is one example that doesn't handle dynamic environment variables well which is why you needed the system property to take priority.

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.

6 participants