Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable ProcessResourceDetector by default #3916

Open
xrmx opened this issue May 10, 2024 · 6 comments
Open

Enable ProcessResourceDetector by default #3916

xrmx opened this issue May 10, 2024 · 6 comments

Comments

@xrmx
Copy link
Contributor

xrmx commented May 10, 2024

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
Copy link
Member

aabmass commented May 16, 2024

@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
Copy link
Contributor Author

xrmx commented May 17, 2024

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
Copy link
Member

aabmass commented May 21, 2024

@xrmx yes it's here

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

@xrmx
Copy link
Contributor Author

xrmx commented May 22, 2024

@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
Copy link
Contributor Author

xrmx commented May 22, 2024

@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
Copy link
Contributor Author

xrmx commented May 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants