search

open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.64k stars 762 forks source link

Implement xray-lambda propagator via the OTEL_PROPAGATORS env variable #4494

Open martinkuba opened 6 months ago

martinkuba commented 6 months ago

The AWS lambda spec recently introduced a new xray-lambda propagator to support AWS Lambda's Active Tracing. This requires not only implementing the new propagator, but also making it possible to configure propagators and their order using the OTEL_PROPAGATORS env variable.

The env variable currently supports only the tracecontext and baggage propagators.

~The approach to make this work for 3rd-party propagators has been discussed in the SIG on 1/31/24, and it has been decided to move all propagators from contrib to the core repo and add them to the BasicTracerProvider lookup.~ In order to support all known propagators (including xray and xray-lambda) in the OTEL_PROPAGATORS environment variable, we have introduced the auto-configuration-propagators package.

Here is a list of tasks to implement this:

Flarna commented 5 months ago

Why is it needed to move the propagators from contrib because of this? I think TracerProvider could just try to require the exporter if it is configured via env and if it is present it's used.

I don't think we should add a dependency to all optional components just to allow to configure them via environment/files.

Users should have the ability to decide which components they install instead of forcing them to install everything.

I wonder about the actual usecase to have xray, B3, W3C, ottrace (...) propagators installed at the same time everywhere and always.

If users want a all propagators package we could create a meta package similar as for autoinstrumentations instead of hardwiring it into the SDK base (or node) package.

martinkuba commented 5 months ago

Why is it needed to move the propagators from contrib because of this?

The reason is that the mapping between the propagator name (e.g. "xray") and the package/class is hard-coded in the TracerProvider. In other words, the TracerProvider needs to know which class to require for the given name. In addition to that, my understanding is that core packages should not have a dependency on contrib packages.

We discussed this in a SIG meeting, and moving these packages was the best solution we could come up with. Note that this really only applies to the propagators that are listed as supported by the OTEL_PROPAGATORS env variable in the spec.

I think TracerProvider could just try to require the exporter if it is configured via env and if it is present it's used.

Do you mean that the mapping between the name and the module is hard-coded in the TracerProvider. Or, can you please elaborate how this might work?

I wonder about the actual usecase to have xray, B3, W3C, ottrace (...) propagators installed at the same time everywhere and always.

This is needed specifically for the lambda use case as described in the spec here. Note that moving the ottrace propagator is not needed for this work, but I included it only for completeness since it is documented as one of the propagators that should be configurable via the environment variable.

Flarna commented 5 months ago

The reason is that the mapping between the propagator name (e.g. "xray") and the package/class is hard-coded in the TracerProvider. In other words, the TracerProvider needs to know which class to require for the given name. In addition to that, my understanding is that core packages should not have a dependency on contrib packages.

This hard coded dependency is problematic in my opinion. In special if is it in BasicTracerProvider it goes also into web and I doubt any bundler will be able to detect which propagator is actually used. Therefore the resulting bundle holds unneeded code. The only way to get rid of them would be to implement your own SDK.

I know propagators are not that big. But I guess similar configs might come for other components like samplers, exporters (e.g see OTEL_TRACES_EXPORTER next to OTEL_PROPAGATORS). Exporters often have further dependencies like grpc,... - so they might be really large.

I think it would be better keep the actual propagator dependencies out of BasicTracerProvider itself but have a config option to add that ones you installed and want to support via env/file config. For sure the SDK Trace package can export well known strings like tracecontext, xray-lambda, ... coming from spec similar as semantic dictionary to avoid duplication. Also basic types (if not in API) should be exported.

Higher layer packages (e.g. NodeSDK,... or some APM vendor bundles or an AWS Lambda monitoring layer) can hide this details and decide for the user to include more out of the box.

This is needed specifically for the lambda use case as described in the spec here.

I don't think hardwiring xray propagator in BasicTracerProvider is needed to fulfill the spec. It's enough to make it flexible enough to allow to configure more propagators. Actually this functionality is already there - NodeTracerProvider adds more propagators (see here). The need to subclass BasicTracerProvider to get this done seems not that user friendly.

Just to be clear, it's not about being against AWS/xray or any other vendor specific topics here. It's all about keeping the base small and flexible.

A bit of topic: I would prefer to remove propagators like jaeger and b3 from NodeTracerProvider for the same reason. But that would be a bit too breaking. Spec just tells that they MUST be distributed via and extension package, not by the SDK itself.

martinkuba commented 5 months ago

@Flarna In general, I agree with you. It would be better to not always distribute all these propagators.

At the same time, the spec does list the known values for the OTEL_PROPAGATORS env variable, which itself is part of the SDK configuration. The xray propagator is part of this list. Even if we include the xray mapping in a higher-level package like the Node SDK, the xray propagator would still need to live here and not in contrib.

The alternative is to not hardcode the mapping anywhere and instead introduce some API that users must call (as you mentioned). Maybe something like

registerPropagator(name: string, propagator: TextMapPropagator);

I am not opposed to this, but it introduces a new pattern, and I am curious what others think. /cc @pichlermarc @dyladan

pichlermarc commented 5 months ago

Even if we include the xray mapping in a higher-level package like the Node SDK, the xray propagator would still need to live here and not in contrib.

I think having it be part of @opentelemetry/sdk-node is the low-friction way of getting something started quickly. Even better if we only conditionally require it. :+1:

I agree that we need to move it to this repo as it's listed as well known by the spec. If we want to conditionally require it from @opentelemetry/sdk-node, we will at least need a devDependency on it.

This hard coded dependency is problematic in my opinion. In special if is it in BasicTracerProvider it goes also into web and I doubt any bundler will be able to detect which propagator is actually used. Therefore the resulting bundle holds unneeded code. The only way to get rid of them would be to implement your own SDK.

I fully agree that it should not go into sdk-trace-base. When I was looking at the issue I must've overlooked that this was the plan - sorry about that. I'm also skeptic of unconditionally including exporters and propagators in the sdk-trace-node package. We've avoided having exporters and propagators in sdk-logs and for sdk-metrics (granted, propagators are not needed there, but exporters are) as we learned from sdk-trace that doing this does not scale for the reasons you mentioned.

In my opinion we should remove the ability to hold exporters and propagators entirely from the Trace SDK package in 2.0

In short I think a well-known propagator should:

That being said, in the future we will need a way to better manage these things as the list of dependencies will just grow and grow from things that are added to the spec. I don't have a well thought-out way to do this - at the moment there are just too many things on my plate to come up with a solution for this, let alone drive the topic. :slightly_frowning_face:

(A not very well though out solution: something like python's opentelemetry-bootstrap may work for us to generate .js/.ts files to automatically set up propagators, instrumentations, exporters and other extensions using an sdk-node-like replacement that we pass things to. But I think any solution to the problem we can come up with here will likely outgrow the scope of this issue.)

github-actions[bot] commented 1 month ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.