open-telemetry / opentelemetry-python

OpenTelemetry Python API and SDK
https://opentelemetry.io
Apache License 2.0
1.67k stars 569 forks source link

Enable ProcessResourceDetector by default #3916

Open xrmx opened 1 month ago

xrmx commented 1 month ago

Is your feature request related to a problem?

It would be helpful to have by default the python version of traced services. The python version is contained in process.runtime attribute: https://opentelemetry.io/docs/specs/semconv/resource/process/

Describe the solution you'd like

Currently the ProcessResourceDetector that is providing the information is not enabled by default but can be enabled via OTEL_EXPERIMENTAL_RESOURCE_DETECTORS environment variable.

From opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py:

       otel_experimental_resource_detectors = environ.get(
            OTEL_EXPERIMENTAL_RESOURCE_DETECTORS, "otel"
        ).split(",")

        if "otel" not in otel_experimental_resource_detectors:
            otel_experimental_resource_detectors.append("otel")

We see that we already enable the OTELResourceDetector even if it the attributes are experimental. So I'd like to add the ProcessResourceDetector enabled by default with something like:

         otel_experimental_resource_detectors = environ.get(
-            OTEL_EXPERIMENTAL_RESOURCE_DETECTORS, "otel"
+            OTEL_EXPERIMENTAL_RESOURCE_DETECTORS, "otel,process"
         ).split(",")

Describe alternatives you've considered

Alternatively if we are worried that sensitive data may be shared from process args we can add by default only a detector providing the process.runtime attributes.

Additional context

aabmass commented 1 month ago

@xrmx what are other languages doing by default? I looked at JS and it is not included with the default Resource. Can you check on Java and Go possibly?

Another concern I had was the prevalence of forking in Python which would invalidate the process.pid. We should ideally fix this before automatically including it in the "stable" default resource.

xrmx commented 1 month ago

For Go is not added by default but instrumentation is only manual AFAIK, for java the same but it's kinda suggested to add it per https://opentelemetry.io/docs/languages/java/resources/

For JS the auto instrumentation package enable it by default: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/metapackages/auto-instrumentations-node/#usage-auto-instrumentation

@aabmass could you please remember me what unique pair you cited in the SIG call?

aabmass commented 1 month ago

@xrmx yes it's here.)

service.namespace,service.name,service.instance.id triplet MUST be globally unique

xrmx commented 1 month ago

@aabmass the service.instance.id is not added by the process resource detector though so with or without the process.pid we have the same issue no?

Anyway I've added a ProcessRuntimeResourceDetector that only sets the process.runtime attributes in my distribution. Would be doing the same acceptable for upstream?

xrmx commented 1 month ago

@aabmass the service.instance.id is not added by the process resource detector though so with or without the process.pid we have the same issue no?

I think I've understood what you meant. The issue is that the process pid is variable while the rest does not change.

xrmx commented 1 month ago

On adding only the process.runtime attributes I see that they are recommended in the spec: https://opentelemetry.io/docs/specs/semconv/resource/process/#process-runtimes

recommended is explained there https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/#recommended