Feat: Add EventToCarrier to AWS Lambda semantic conventions #164

rapphil · 2023-07-06T04:46:21Z

This PR is an attempt to create a long term solution for addressing this issue that is affecting the opentelemetry-lambda: open-telemetry/opentelemetry-lambda#714

This PR:

Introduces the EventToCarrier entity, that allows you to convert a lambda event into a carrier to be used in TextMapPropagators to extract the context.
Proposes an environment variable OTEL_AWS_LAMBDA_EVENT_TO_CARRIERS to be used in lambdas to control what EventToCarriers should be used in the lifecycle of a lambda.

ps.: I decided to remove the span links because they were only inserted as workaround for the problem of not being possible to use different context propagation mechanisms. I'm open to discussing adding it back with a more configurable behaviour.

There is a proof of concept java implementation in this PR: rapphil/opentelemetry-java-instrumentation#1 (I'm not creating a PR against the otel repo to reduce the noise).

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Oberon00 · 2023-07-06T08:20:46Z

specification/faas/aws-lambda.md


-[Span Link]: https://opentelemetry.io/docs/concepts/signals/traces/#span-links
+The `EventToCarrier` MUST implement the `Convert` operation to convert a lammbda `Event` into a `Carrier`.


This EventToCarrier sounds like an ordinary getter function that can be passed to the propagator along with the carrier. And in fact that is how it is implemented in some places, e.g. see .NET https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/Instrumentation.AWSLambda-1.1.0-beta.3/src/OpenTelemetry.Instrumentation.AWSLambda/Implementation/AWSLambdaUtils.cs#L68-L95

So I would suggest to change the wording here to avoid introducing this new intermediate concept.

I disagree. The linked .NET implementation does not appear to be user-provided and thus does not account for any event type not known to the instrumentation. It also does not provide a mechanism for extracting context information from any source other than the specific sources it has implemented. Because it combines the context propagation and event processing it does not provide any opportunity for composition to enable the user to establish a chain of responsibility. In fact, it's not at all implemented as an ordinary getter function that can be passed to a propagator, it is a sealed context extraction mechanism with limited knowledge of how to process a few event types.

This PR is also not proposing anything user-configurable beyond the sealed list of strings. If it does, please clarify the wording, or point me to where I missed it.

(Regarding the particular linked instrumentation, a custom context can be extracted by the user and then passed explicitly https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/Instrumentation.AWSLambda-1.1.0-beta.3/src/OpenTelemetry.Instrumentation.AWSLambda/AWSLambdaWrapper.cs#L97 which is much simpler to use, implement & understand in this case (but of course the concept would not be applicable to fully automatic / agent based instrumentations).)

In the new revision I tried to make it clear that the EventToCarrier is configurable.

Oberon00 · 2023-07-06T08:25:37Z

specification/faas/aws-lambda.md

+* `http_headers` = populates the `Carrier` with the content of the http headers.
+* `sqs` - populate the carrier with the content of the `AWSTraceHeader` system attribute of the message.


This is a bit unclear/inconsistent. For the HTTP headers, you would also have the headers like traceparent, etc. available that may be usable with propagators other than X-Ray. For SQS you specify only the system attributes, which will only contain X-Ray.

Agreed, extracting a carrier from an SQS event should probably include both message attributes and message system attributes.

I removed the list of pre defined EventToCarrier from the new revision.

Oberon00 · 2023-07-06T08:29:13Z

specification/faas/aws-lambda.md

+
+Valid values to configure the composite `EventToCarrier` are:
+
+* `lambda_runtime` - populates the `Carrier` with a key `X-Amzn-Trace-Id` from the value of the `_X_AMZN_TRACE_ID` environment variable. (see note below)


I think there is actually no need to have a per-trigger-type distinction, since most functions will only have a single trigger type anyway. I think instead there are 3 things for which it may make sense to switch them on/off separately:

Propagation using X-Ray from _X_AMZN_TRACE_ID (or equivalent Java sytem property)

Propagation using X-Ray information in event payload (e.g. SQS system attributes, HTTP headers)

Propagation using the default / configured propagator using sensible locations in the event payload (e.g. SQS message attributes, SNS message attributes, HTTP headers)

This is not a per-trigger-type distinction, this is enumerating a set of pre-defined implementations of the EventToCarrier abstraction that can be selected at runtime.

As for the three toggles you propose, two and three are the same thing as long as a carrier can be extracted from the event and handed to the configured propagator. One is also effectively the same to the extent that an EventToCarrier implementation can always choose to include information in the carrier that was not in the incoming event but instead extracted from the operating environment or some other source.

The whole point of this is to separate the "Propagation using " from the second half of each of those sentences and to give the user the controls they need to choose what works best for them. We don't need to choose only three options for the user.

Please note that in all Lambda instrumentations I have seen that use X-Ray-dependent inputs, they also use an hard-coded X-Ray propagator, disregarding any configured propagator, which is sensible.

This is not a per-trigger-type distinction, this is enumerating a set of pre-defined implementations of the EventToCarrier abstraction that can be selected at runtime.

I don't really get the difference, but what I meant is: There is no reason to make the user manually select "http" vs "sqs" when the event type can be automatically determined either from the function signature or by inspecting the payload JSON. Selecting them separately would only make sense if you have a function that receives both http and sqs events (not easily possible in statically typed languages like Java, .NET, but possible in priciple -- still probably unusual) and want to only extract context from one of them (I can't really come up with a reasonable use case for that)

I fully support not requiring the user to configure something that can be determined automatically. As such, separating sqs and http seems unnecessary.

specification/faas/aws-lambda.md

Aneurysm9 · 2023-07-06T15:23:16Z

specification/faas/aws-lambda.md


-[Span Link]: https://opentelemetry.io/docs/concepts/signals/traces/#span-links
+The `EventToCarrier` MUST implement the `Convert` operation to convert a lammbda `Event` into a `Carrier`.


I disagree. The linked .NET implementation does not appear to be user-provided and thus does not account for any event type not known to the instrumentation. It also does not provide a mechanism for extracting context information from any source other than the specific sources it has implemented. Because it combines the context propagation and event processing it does not provide any opportunity for composition to enable the user to establish a chain of responsibility. In fact, it's not at all implemented as an ordinary getter function that can be passed to a propagator, it is a sealed context extraction mechanism with limited knowledge of how to process a few event types.

Aneurysm9 · 2023-07-06T15:27:04Z

specification/faas/aws-lambda.md

+The `EventToCarrier` MUST implement the `Convert` operation to convert a lammbda `Event` into a `Carrier`.
+
+The `Convert` operation MUST have the following parameters:
+  `Carrier` - the carrier that will be populated from the `Event`


Why must the caller provide the carrier to populate? Should this not be a pure function of event -> carrier?

Aneurysm9 · 2023-07-06T15:32:53Z

specification/faas/aws-lambda.md

+
+Implementations MUST provide a facility to group multiple `EventToCarrier`s. A composite `EventToCarrier` can be built from a list of `EventToCarrier`s. The resulting composite `EventToCarrier` will invoke the `Convert` operation of each individual `EventToCarrier` in the order they were specified, sequentially updating the carrier.
+
+The list of `EventToCarrier`s passed to the composite `EventToCarrier` MUST be configured using the `OTEL_AWS_LAMBDA_EVENT_TO_CARRIERS`, as a comma separated list of values.


Why? New environment-based configuration cannot be added to the specification at this time.

At the very least this needs to be reduced to SHOULD, but I think it should be removed entirely.

Does OTEL_AWS_LAMBDA_EVENT_TO_CARRIERSenvironment variable also serve the purpose of exposing user-configurable event carrier ask from @Oberon00 comment above? https://github.com/open-telemetry/semantic-conventions/pull/164/files#r1254641850

If that is the case we should keep this as is or provide some configuration if not already provided (same ask in
@Oberon00in comment )

Aneurysm9 · 2023-07-06T15:36:13Z

specification/faas/aws-lambda.md

+
+The list of `EventToCarrier`s passed to the composite `EventToCarrier` MUST be configured using the `OTEL_AWS_LAMBDA_EVENT_TO_CARRIERS`, as a comma separated list of values.
+
+Valid values to configure the composite `EventToCarrier` are:


Suggested change

Valid values to configure the composite `EventToCarrier` are:

Valid values to configure the composite `EventToCarrier` include:

There may be reasons for other values to also be valid.

Aneurysm9 · 2023-07-06T15:40:00Z

specification/faas/aws-lambda.md

+
+Valid values to configure the composite `EventToCarrier` are:
+
+* `lambda_runtime` - populates the `Carrier` with a key `X-Amzn-Trace-Id` from the value of the `_X_AMZN_TRACE_ID` environment variable. (see note below)


This is not a per-trigger-type distinction, this is enumerating a set of pre-defined implementations of the EventToCarrier abstraction that can be selected at runtime.

As for the three toggles you propose, two and three are the same thing as long as a carrier can be extracted from the event and handed to the configured propagator. One is also effectively the same to the extent that an EventToCarrier implementation can always choose to include information in the carrier that was not in the incoming event but instead extracted from the operating environment or some other source.

The whole point of this is to separate the "Propagation using " from the second half of each of those sentences and to give the user the controls they need to choose what works best for them. We don't need to choose only three options for the user.

Aneurysm9 · 2023-07-06T15:42:35Z

specification/faas/aws-lambda.md

+* `http_headers` = populates the `Carrier` with the content of the http headers.
+* `sqs` - populate the carrier with the content of the `AWSTraceHeader` system attribute of the message.


Agreed, extracting a carrier from an SQS event should probably include both message attributes and message system attributes.

specification/faas/aws-lambda.md

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

…lemetry#165)

…cs` (open-telemetry#166)

…nv (open-telemetry#171) Co-authored-by: Joao Grassi <joao@joaograssi.com> Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

…-telemetry#170) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

…eck (open-telemetry#177) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

Signed-off-by: YANGDB <yang.db.dev@gmail.com> Co-authored-by: Josh Suereth <joshuasuereth@google.com>

…etry#175)

…y#180)

open-telemetry#187)

open-telemetry#190) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

Co-authored-by: Josh Suereth <joshuasuereth@google.com>

…pen-telemetry#99) Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com> Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

…#224)

…#228)

…emetry#156) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

…metry#241)

Signed-off-by: Raphael Silva <rapphil@gmail.com>

* Clarify that users can provider their own EventToCarrier * Dont define the list of implementations * Remove the composite EventToCarrier

tylerbenson · 2023-08-14T15:36:10Z

docs/faas/aws-lambda.md

+
+Lambda instrumentation MUST provide a default `EventToCarrier`, the `HttpEventToCarrier`, which populates the `Carrier` with the content of the http headers used to invoke the lambda function.
+
+If no `EventToCarrier` is provided during the initialization of the lambda instrumentation, the `HttpEventToCarrier` MUST be used.


This doesn't take @Oberon00's feedback into account. As written, it seems that SQS lambda's will require explicit config to select the proper EventToCarrier implementation. This should not be required of users.

I see, is there anything in the proposal blocking the implementation of a EventToCarrier that is supposed to work with SQS? I don't think we need to list all possible cases and how they should be implemented, do we?

The important thing is that the lambda instrumentation can receive an EventToCarrier as parameter.

Listing cases that COULD be supported was a source of confusion in the previous revision of this proposal.

The problem is not missing cases, but that the user should not need to configure this explicitly. The implementation should automatically extract without the user configuring the trigger type explicitly.

jsuereth · 2023-10-23T17:07:38Z

Closing this in favor of ongoing discussion in #354.

Feat: Add EventToCarrier to AWS Lambda semantic conventions

3aaa4cd

Signed-off-by: Raphael Silva <rapphil@gmail.com>

rapphil requested review from a team July 6, 2023 04:46

github-actions bot assigned reyang Jul 6, 2023

rapphil added 3 commits July 6, 2023 04:48

Fix: fix TOC

526dc65

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Fix: fix toc

ab15449

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Merge branch 'main' into rapphil-add-event-to-carrier

8606301

Oberon00 self-requested a review July 6, 2023 08:16

Oberon00 reviewed Jul 6, 2023

View reviewed changes

Aneurysm9 reviewed Jul 6, 2023

View reviewed changes

tylerbenson reviewed Jul 6, 2023

View reviewed changes

specification/faas/aws-lambda.md Outdated Show resolved Hide resolved

tylerbenson mentioned this pull request Jul 17, 2023

AWS X-Ray Environment Variable Propagation open-telemetry/opentelemetry-specification#3605

Closed

dyladan mentioned this pull request Aug 7, 2023

feat(instrumentation-aws-lambda): Changed capturing of X-Ray context as span link open-telemetry/opentelemetry-js-contrib#1411

Closed

rapphil and others added 15 commits August 13, 2023 22:39

Update specification/faas/aws-lambda.md

29fdde3

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Moved resource semconv directly under the resource directory (open-te…

71f4932

…lemetry#165)

Renamed specification and semantic_conventions to model and `do…

f4a5387

…cs` (open-telemetry#166)

Fix path of md table generation make task (open-telemetry#173)

053fe9a

Add markdown file for url semantic conventions (open-telemetry#174)

86940d2

[editorial] Add missing README.md for Resource Cloud Provider semco…

c2b9767

…nv (open-telemetry#171) Co-authored-by: Joao Grassi <joao@joaograssi.com> Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

[editorial] docs/rpc/json-rpc: fix title and in-page hypenation (open…

02071de

…-telemetry#170) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

[editorial] Rename docs files and folders: replace "_" by "-"; add ch…

57bd011

…eck (open-telemetry#177) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

Fix semantic-conventions README.md file links (open-telemetry#168)

c35b1d4

Signed-off-by: YANGDB <yang.db.dev@gmail.com> Co-authored-by: Josh Suereth <joshuasuereth@google.com>

[editorial] Fix doc page titles and add Hugo front matter (open-telem…

aa85e62

…etry#175)

[editorial] Link directly to page, not to page's title (open-telemetr…

31ca6ef

…y#180)

Bump to latest version of the specification. (open-telemetry#185)

f875722

[CI] Report link-check error when external URL used for local doc page (

5200be9

open-telemetry#187)

Update documentation for how to cut a release AND perform the action … (

69f6bae

open-telemetry#190) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

[editorial] markdownlint in less then 1.5 sec (open-telemetry#193)

12bd6b0

jsuereth and others added 17 commits August 14, 2023 05:44

Re-enable the schema check to look at the website. (open-telemetry#217)

a46db0f

Add Alexander Wert as an Approver (open-telemetry#220)

1c4e58f

Bump semantic conventions tooling to v0.20.0 (open-telemetry#225)

ff064a4

Co-authored-by: Josh Suereth <joshuasuereth@google.com>

Add PR template (open-telemetry#223)

740ebd6

Generate database metrics semconv from YAML (open-telemetry#90)

6561f68

Generate RPC metrics from YAML (open-telemetry#93)

a491f82

Rename http.*.duration to http.*.request.duration (open-telemetry…

790eac9

…#224)

HTTP version should be 2 and 3 instead of 2.0 and 3.0 (open-telemetry…

2f4d8b4

…#228)

Generate FaaS metrics semconv from YAML (open-telemetry#88)

94b653e

Re-introduce namespace to describe the original destination (open-tel…

8dfd92c

…emetry#156) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

Move Joao Grassi from approver to maintainer (open-telemetry#239)

50cf594

Update destination attribute briefs (open-telemetry#243)

d550c98

Rename all JVM metrics from process.runtime.jvm.* to jvm.* (open-tele…

a6c3202

…metry#241)

Fix: fix TOC

92ad618

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Fix: fix toc

1d3b5ca

Signed-off-by: Raphael Silva <rapphil@gmail.com>

Update EvenToCarrier based on feedback

58c722c

* Clarify that users can provider their own EventToCarrier * Dont define the list of implementations * Remove the composite EventToCarrier

rapphil requested review from a team August 14, 2023 06:27

rapphil requested a review from tigrannajaryan as a code owner August 14, 2023 06:27

Merge branch 'main' into rapphil-add-event-to-carrier

7fd5a23

rapphil requested review from Oberon00, tylerbenson and pavankrish123 August 14, 2023 15:31

tylerbenson reviewed Aug 14, 2023

View reviewed changes

rapphil mentioned this pull request Aug 22, 2023

FaaS: Change requirements regarding handling of AWS Lambda-provided SpanContext (take 2) #277

Closed

rapphil added 2 commits September 11, 2023 21:24

Update to simplify language

94a3bf5

Merge branch 'main' into rapphil-add-event-to-carrier

6dd68fe

jsuereth closed this Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add EventToCarrier to AWS Lambda semantic conventions #164

Feat: Add EventToCarrier to AWS Lambda semantic conventions #164

rapphil commented Jul 6, 2023 •

edited

Loading

Oberon00 Jul 6, 2023 •

edited

Loading

Aneurysm9 Jul 6, 2023

Oberon00 Jul 6, 2023

rapphil Aug 14, 2023

Oberon00 Jul 6, 2023

Aneurysm9 Jul 6, 2023

rapphil Aug 14, 2023

Oberon00 Jul 6, 2023 •

edited

Loading

Aneurysm9 Jul 6, 2023

Oberon00 Jul 6, 2023

tylerbenson Jul 6, 2023

Aneurysm9 Jul 6, 2023

Aneurysm9 Jul 6, 2023

Aneurysm9 Jul 6, 2023

pavankrish123 Jul 10, 2023 •

edited

Loading

Aneurysm9 Jul 6, 2023

Aneurysm9 Jul 6, 2023

Aneurysm9 Jul 6, 2023

tylerbenson Aug 14, 2023

rapphil Aug 14, 2023

Oberon00 Sep 12, 2023

jsuereth commented Oct 23, 2023


		[Span Link]: https://opentelemetry.io/docs/concepts/signals/traces/#span-links
		The `EventToCarrier` MUST implement the `Convert` operation to convert a lammbda `Event` into a `Carrier`.

		* `http_headers` = populates the `Carrier` with the content of the http headers.
		* `sqs` - populate the carrier with the content of the `AWSTraceHeader` system attribute of the message.


		Valid values to configure the composite `EventToCarrier` are:

		* `lambda_runtime` - populates the `Carrier` with a key `X-Amzn-Trace-Id` from the value of the `_X_AMZN_TRACE_ID` environment variable. (see note below)


		Implementations MUST provide a facility to group multiple `EventToCarrier`s. A composite `EventToCarrier` can be built from a list of `EventToCarrier`s. The resulting composite `EventToCarrier` will invoke the `Convert` operation of each individual `EventToCarrier` in the order they were specified, sequentially updating the carrier.

		The list of `EventToCarrier`s passed to the composite `EventToCarrier` MUST be configured using the `OTEL_AWS_LAMBDA_EVENT_TO_CARRIERS`, as a comma separated list of values.


		Lambda instrumentation MUST provide a default `EventToCarrier`, the `HttpEventToCarrier`, which populates the `Carrier` with the content of the http headers used to invoke the lambda function.

		If no `EventToCarrier` is provided during the initialization of the lambda instrumentation, the `HttpEventToCarrier` MUST be used.

Feat: Add EventToCarrier to AWS Lambda semantic conventions #164

Feat: Add EventToCarrier to AWS Lambda semantic conventions #164

Conversation

rapphil commented Jul 6, 2023 • edited Loading

Oberon00 Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavankrish123 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuereth commented Oct 23, 2023

rapphil commented Jul 6, 2023 •

edited

Loading

Oberon00 Jul 6, 2023 •

edited

Loading

Oberon00 Jul 6, 2023 •

edited

Loading

pavankrish123 Jul 10, 2023 •

edited

Loading