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.

Sign up for GitHub

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

Jump to bottom

Updated Documentation For Change in Behaviour of Reading X-Ray Tracing Info From Lambda #3502

Closed
wants to merge 5 commits into from
Closed

Updated Documentation For Change in Behaviour of Reading X-Ray Tracing Info From Lambda #3502

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e89a8ee
updated documentation to match pr 8368 read tracing info from system …
jdoherty May 12, 2023
5c3effe
Merge branch 'main' of https://github.com/jdoherty/opentelemetry-spec…
jdoherty May 12, 2023
0a536f8
updated following PR
jdoherty May 12, 2023
f4a213a
updating to fix linting error
jdoherty May 12, 2023
8833cbd
removing system properties link
jdoherty May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions specification/trace/semantic_conventions/instrumentation/aws-lambda.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,17 @@ and the [cloud resource conventions][cloud]. The following AWS Lambda-specific a

### AWS X-Ray Environment Span Link

If the `_X_AMZN_TRACE_ID` environment variable is set, instrumentation SHOULD try to parse an
OpenTelemetry `Context` out of it using the [AWS X-Ray Propagator](../../../context/api-propagators.md). If the
resulting `Context` is [valid](../../api.md#isvalid) then a [Span Link][] SHOULD be added to the new Span's
[start options](../../api.md#specifying-links) with an associated attribute of `source=x-ray-env` to
indicate the source of the linked span.
Instrumentation MUST check if the context is valid before using it because the `_X_AMZN_TRACE_ID` environment variable can
contain an incomplete trace context which indicates X-Ray isn’t enabled. The environment variable will be set and the
There are two common ways to read the tracing information if AWS X-Ray is enabled for a lambda function:

1. Via the system property `com.amazonaws.xray.traceHeader`.
Copy link
Member

Choose a reason for hiding this comment

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

As this is language-independent spec, you need to add more context here on what a "system property" is. I believe this is something specific to the Java runtime?

Copy link
Author

Choose a reason for hiding this comment

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

ok sure - I have updated this now - is this any better?

Copy link
Member

Choose a reason for hiding this comment

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

Linking to the docs of system properties seems a bit overkill, probably any Java programmer should know them. I think just mentioning that it is Java specific is enough. If linking anything, maybe https://docs.aws.amazon.com/lambda/latest/dg/lambda-java.html

Copy link
Author

Choose a reason for hiding this comment

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

ok sure I have removed this now

2. Via an environment variable that is set, titled `_X_AMZN_TRACE_ID`.

If the system property `com.amazonaws.xray.traceHeader` is set, instrumentation SHOULD try to parse an OpenTelemetry `Context` out of it using the [AWS X-Ray Propagator](../../../context/api-propagators.md).
Alternatively, if the system property is not set then instrumentation SHOULD try to parse an OpenTelemetry `Context` out of the `_X_AMZN_TRACE_ID` environment variable using the [AWS X-Ray Propagator](../../../context/api-propagators.md).
If the resulting `Context` is [valid](../../api.md#isvalid) then a [Span Link][] SHOULD be added to the new Span's
[start options](../../api.md#specifying-links) with an associated attribute of `source=x-ray-env` to indicate the source of the linked span.
Copy link
Member

Choose a reason for hiding this comment

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

Such a link attribute should have a proper semantic convention YAML entry (type attribute_group since we don't have a specific type for links yet).

I expect this will also lead to larger discussions, e.g. whether an unprefixed attribute is OK, or which prefix to use, or, since this seems to be a generally useful attribute, whether we want something similar for the span parent.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry Christian - I'm not quite sure what you mean here - can you elaborate on this?

Copy link
Member

Choose a reason for hiding this comment

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

When the spec recommends to set an attribute with a particular name, we always have a corresponding entry in the semantic conventions which are at just this moment moving from this repository to https://github.com/open-telemetry/semantic-conventions.

Adding them there will make many OpenTelemetry languages (semi-)automatically generate string constants with the keys and documentation comments.

Copy link
Author

Choose a reason for hiding this comment

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

Ok understood - can this be tackled in another PR? This PR is mainly to cover the update to the docs to cover the change made here:

open-telemetry/opentelemetry-java-instrumentation#8368

Instrumentation MUST check if the context is valid before using it because the `_X_AMZN_TRACE_ID` environment variable or the system property `com.amazonaws.xray.traceHeader` can
contain an incomplete trace context which indicates X-Ray isn’t enabled. The environment variable or system property will be set and the
Copy link
Member

Choose a reason for hiding this comment

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

This seems redundant with the next sentence

Copy link
Author

Choose a reason for hiding this comment

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

ok sure - I have now removed this

`Context` will be valid and sampled only if AWS X-Ray has been enabled for the Lambda function. A user can
disable AWS X-Ray for the function if the X-Ray Span Link is not desired.

Expand Down