open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.75k stars 889 forks source link

Add support for `OTEL_RESOURCE_DETECTORS` environment variable #2948

Open srikanthccv opened 2 years ago

srikanthccv commented 2 years ago

I looked around and didn't find anything related to this. Feel free to point me out if this has been discussed earlier. The SDK default behaviour is to use EnvDetector (using env OTEL_RESOURCE_ATTRIBUTES) and populate some necessary default values. There is no way to configure the SDK to detect and load the resource detector automatically. This is especially important in auto instrumentation cases where the user also wants some custom/third-party resource detector configured via env as with other components such as exporters. I propose adding OTEL_RESOURCE_DETECTORS with default value env and (optionally cloud platform-specific standard detectors). I can send a PR if this is accepted.

The original issue that motivated this https://github.com/open-telemetry/opentelemetry-python-contrib/issues/1372

sanketmehta28 commented 1 year ago

Hi @open-telemetry/specs-triagers : any update on this issue?

reyang commented 1 year ago

FYI https://github.com/open-telemetry/opentelemetry-specification/issues/2891#issuecomment-1289241503.

reyang commented 1 year ago

The OTEL_RESOURCE_DETECTORS violates "Anything that has a non-primitive type (see here for the list of primitive types)"

srikanthccv commented 1 year ago

@reyang, I am not sure I understood it. How is this different from OTEL_TRACES_EXPORTER or any other such variable? How do they not violate the spec, and this does? I am proposing that users configure the name of the resource detector similar to how they would confirm the exporter's name to use.

reyang commented 1 year ago

It is DETECTOR or DETECTORS?

srikanthccv commented 1 year ago

I am okay with either of the naming but what I am mainly trying to achieve is the SDK to support this capability. An EnvDetector is mandated by spec, which reads the OTEL_RESOURCE_ATTRIBUTES and returns a Resource. There is no way to configure the SDK with additional custom Resources with env. This is a proposal aimed at addressing that gap.

svrnm commented 1 year ago

I added a comment to https://github.com/open-telemetry/opentelemetry-python-contrib/issues/1372 already, that OTEL_RESOURCE_DETECTORS might not work as expected:

I argue that the way how resource detectors are specified today in the spec, is not practical to end-users: right now the burden to decide which resource detectors are part of an application lays with the developer, although the developer might not have a full understanding on which resources an application will run in the future, e.g. the application might be migrated from one environment to another (let's say from Cloud Vendor A to Cloud Vendor B), or the application might be an open source product used in plenty of environments, like bare metal, docker, different cloud vendors, etc. There are now two options:

So, the question I am asking myself now, is how can OpenTelemetry provide a practical way for Developers & Operators to detect resources?

srikanthccv commented 1 year ago

All the exporters are also outside of the SDK and implemented as standalone packages. The propagator such as jaeger and b3 are separate and not included in SDK by default but can still be configured with OTEL_PROPAGATORS using comma delimited values like baggage,jaeger,b3. And I am proposing to provide the same capability for the Resources.

svrnm commented 1 year ago

All the exporters are also outside of the SDK and implemented as standalone packages. The propagator such as jaeger and b3 are separate and not included in SDK by default but can still be configured with OTEL_PROPAGATORS using comma delimited values like baggage,jaeger,b3. And I am proposing to provide the same capability for the Resources.

Fair point, I had to think about this for a while. From that perspective I agree that having OTEL_RESOURCE_DETECTORS is a meaningful addition to the spec (although I assume the addition is blocked by the moratorium @reyang mentioned here).

Independent of that end-users might need some guidance on which detectors to add in which situation, but that's an issue on it's own.

joaopgrassi commented 1 year ago

The propagator such as jaeger and b3 are separate and not included in SDK by default

Arent' these included by default in the API though?

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#propagators-distribution

srikanthccv commented 1 year ago

Arent' these included by default in the API though?

Only tracecontext and baggage are included by default in API. The remaining propagators such jaeger,b3, and ottrace are still provided by OpenTelemetry as extension packages. I am not familiar with all SDKs but this is the case for go, py, java, and js.

joaopgrassi commented 1 year ago

Alright :). Remembered I saw the b3 in the API somewhere, but found it: It's in the .NET API package https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Api/Context/Propagation as has been deprecated so all good.

srikanthccv commented 1 year ago

@reyang, do you still think this should be rejected? I would appreciate the reasoning behind the rejection, given that we have similar capabilities supported for exporters/propagators etc...

reyang commented 1 year ago

@srikanthccv the reason is captured here.