opentelemetry-python icon indicating copy to clipboard operation
opentelemetry-python copied to clipboard

Enable ProcessResourceDetector by default

Open xrmx opened this issue 1 year ago • 6 comments

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

xrmx avatar May 10 '24 10:05 xrmx

@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.

aabmass avatar May 16 '24 17:05 aabmass

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?

xrmx avatar May 17 '24 07:05 xrmx

@xrmx yes it's here

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

aabmass avatar May 21 '24 15:05 aabmass

~~@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 avatar May 22 '24 09:05 xrmx

@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 avatar May 22 '24 09:05 xrmx

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

xrmx avatar May 23 '24 16:05 xrmx