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

Resource Detection for container properties (e.g.container.id) #1372

Closed
svrnm opened this issue Oct 4, 2022 · 18 comments · Fixed by #1584
Closed

Resource Detection for container properties (e.g.container.id) #1372

svrnm opened this issue Oct 4, 2022 · 18 comments · Fixed by #1584
Assignees

Comments

@svrnm
Copy link
Member

svrnm commented Oct 4, 2022

Is your feature request related to a problem?

Detecting the id of the container that holds the service instrumented with otel allows end-users to correlate container issues with service issues.

Describe the solution you'd like
I see some of that implemented in the AWS Extension, but I was wondering if there's a way to have this standalone.

Describe alternatives you've considered
Injecting the container id via an environment variable is always possible.

Additional context
Semantic conventions for container resource: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/container.md

A similar implementation exists for Java, JS, .NET & Go already. The .JS is the most advanced since it also works with containers when cgroup v2 is used:

https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts

We are also currently trying at opentelemetry.io to have per-language pages on resources and used the container detection as an example besides using the environment variable:

open-telemetry/opentelemetry.io#1773
and
https://opentelemetry.io/docs/instrumentation/js/resources/

@sanketmehta28
Copy link
Member

This seems to be a good feature request.
Here is my approach to implement this feature:

  • create a ContainerResourceDetector class in a separate module which extends ResourceDetector class from API and override detect() method which returns Resource obj.

  • in each framework, set the tracer_provider like this:

trace.set_tracer_provider(
        TracerProvider(
            resource=get_aggregated_resources(
                [
                    ContainerResourceDetector(),
                ]
            ),
        )
    )

There is an issue with this approach:

  • It should work when tracer_provider is not provided before set_tracer_provider() is called in a framework.
    But in case where tracer_provider is created outside and then passed to the Instrumentor, there is no way to set the tracer.resource with my resources (collected by ContainerResourceDetector).

Can anyone help me with this issue?

@sanketmehta28 sanketmehta28 self-assigned this Nov 17, 2022
@sanketmehta28
Copy link
Member

sanketmehta28 commented Nov 17, 2022

This is the comment I have received from @srikanthccv for the above issue:

It is not allowed to be set at a later point in time, but you could implement an entry point kind of thing. I realized this is not supported yet by SDK and spec so I opened [this issue]( https://github.com/open-telemetry/opentelemetry-specification/issues/2948) I think this is an important discussion that should take place in the issue itself for broad visibility.

@sanketmehta28
Copy link
Member

@ocelotl : tagging you here as well

@srikanthccv
Copy link
Member

The ResourceDetection is independent of library instrumentation. You don't need all these issues opened for each framework. Users don't set the tracer provider in each framework separately. It happens once, and the user is expected to provide the ContainerResourceDetector when setting the provider manually and via the env(achieved through the entry point) when done through auto instrumentation.

@sanketmehta28
Copy link
Member

Hi @srikanthccv , In case of user is not providing the tracer_provider, I was thinking to create one inside _instrument() function with ContainerResourceDetector passed as argument so that this container.id will automatically be available in spans.

I understand that ResourceDetector module is independent of library instrumentation but I wanted to use this module in each framework inside instrumentation only. That was the reason why I had created separate issues for each framework.

Also in case of ENV variable approach there are 2 problems:

  • specification may/may not approve it as it should be compatible to other SIGs as well or it can take a long time before they do it. (Diego also agreed to this point) What is the alternate option in that case?
  • if we specify the OTEL_RESOURCE_DETECTORS=ContainerResourceDetector in that case, we have to parse this ENV variable to create object of that class and call its detect() in Resource.Create() in SDK when the actual implementation lies in contrib repo. I believe this is not the right approach.

Please do let me know if you think I have misunderstood anything here.

@srikanthccv
Copy link
Member

I was thinking to create one inside _instrument() function with ContainerResourceDetector passed as argument so that this container.id will automatically be available in spans

No, the instrumentation will not set any (tracer/meter/logger)provider on its own for any reason. In the absence of a tracer provider, it will be all no-op, and it's a design decision.

but I wanted to use this module in each framework inside instrumentation only. That was the reason why I had created separate issues for each framework.

Resource is an SDK component, and no SDK related things will be part of the instrumentation.

specification may/may not approve it as it should be compatible to other SIGs as well or it can take a long time before they do it.

If you are referring OTEL_RESOURCE_DETECTORS, I can't think of a reason why it will not be accepted.

What is the alternate option in that case?

I don't know of any alternative at the moment. And we don't want to do something that is not spec-compliant. You are free to propose a solution.

if we specify the OTEL_RESOURCE_DETECTORS=ContainerResourceDetector in that case, we have to parse this ENV variable to create object of that class and call its detect() in Resource.Create() in SDK when the actual implementation lies in contrib repo. I believe this is not the right approach

Why do you believe this is not the right approach?

@svrnm
Copy link
Member Author

svrnm commented Nov 21, 2022

Hey @sanketmehta28 , @srikanthccv , thanks for discussing this. Since I raised this issue a few comments on that matter:

A few other language implementations have the support for resource detectors already in place, and took the following approaches to get them loaded:

So independent of the OTEL_RESOURCE_DETECTORS, I see two things that python SIG could implement right now and being spec compliant & consistent with other languages:

  • Have a way to add resource detectors to the SDK initialisations
  • Have a bunch of resource detectors included in the distro/auto instrumentation

Regarding OTEL_RESOURCE_DETECTORS: I assume that Sanket's concern is the following: If I define resource detectors in that list, they have to be part of the application I am using, e.g. the developer made the deliberate choice to add the ContainerResourceDetector (or others) to the application while creating it. I will comment on the same matter at the spec issue: I think it's worth to go back a few steps and re-think how resource detectors are added.

@srikanthccv
Copy link
Member

Have a way to add resource detectors to the SDK initialisations

This is already supported.

Have a bunch of resource detectors included in the distro/auto instrumentation

By included, you mean they get installed as a part of the distro; it's fine we do this already for instrumentations but including everything, as in all attribute data users have not opted for, is not a good thing. There will also be third-party resource detectors outside this repo which we have no control over, and there needs to be a mechanism to configure them, which is where I bring the feature request to add new env.

Regarding OTEL_RESOURCE_DETECTORS: I assume that Sanket's concern is the following: If I define resource detectors in that list, they have to be part of the application I am using, e.g. the developer made the deliberate choice to add the ContainerResourceDetector (or others) to the application while creating it.

I don't get this argument. Let's take what's already spec'd out, When the user configures OTEL_TRACES_EXPORTER=otlp,jarger,zipkin then all these exporters have to be part of the application, and it's the deliberate choice they are making. The feature request to add OTEL_RESOURCE_DETECTORS is nothing different. It's also the same thing that makes OTEL_PROPAGATORS work.

@svrnm
Copy link
Member Author

svrnm commented Nov 21, 2022

Have a bunch of resource detectors included in the distro/auto instrumentation

By included, you mean they get installed as a part of the distro; it's fine we do this already for instrumentations but including everything, as in all attribute data users have not opted for, is not a good thing. There will also be third-party resource detectors outside this repo which we have no control over, and there needs to be a mechanism to configure them, which is where I bring the feature request to add new env.

I don't care that much about third-party (aka cloud vendors) resource detectors and more about "standard" ones like container , k8s or other common infrastructure.

I don't get this argument. Let's take what's already spec'd out, When the user configures OTEL_TRACES_EXPORTER=otlp,jarger,zipkin then all these exporters have to be part of the application, and it's the deliberate choice they are making. The feature request to add OTEL_RESOURCE_DETECTORS is nothing different. It's also the same thing that makes OTEL_PROPAGATORS work.

I see that you commented on my more lengthy comment over at the spec as well, so let's move that conversation there?

@srikanthccv
Copy link
Member

I don't care that much about third-party (aka cloud vendors) resource detectors and more about "standard" ones like container , k8s or other common infrastructure.

From a design perspective, we want a solution that works well for the "standard", or otherwise, that doesn't require rethinking when users come to ask for support for the vendor or even their custom detectors.

I see that you commented on my more lengthy comment over at the spec as well, so let's move that conversation there?

Sure, I am conveying this is not a new problem - we already have this addressed for other components and let's follow the pattern.

@sanketmehta28
Copy link
Member

sanketmehta28 commented Nov 21, 2022

Have a bunch of resource detectors included in the distro/auto instrumentation

@srikanthccv , @svrnm : Independent of the ENV var OTEL_RESOURCE_DETECTORS, how do you plan to use these detectors in auto-instrumentation? an example will be of great help here.

@ocelotl
Copy link
Contributor

ocelotl commented Nov 21, 2022

Just for the record, discussed in our last SIG, @sanketmehta28 will look into other SIGs implementations maybe they have a way to solve this problem that can work for us 

@sanketmehta28
Copy link
Member

sanketmehta28 commented Dec 1, 2022

Hi All,
Here is the status of containerDetector feature for other SIGS:

NodeJS: Not available in auto instrumentation

DotNet: Not available in auto instrumentation

Java: available in auto instrumentation

GoLang: Not available in auto instrumentation

By looking at the implementation and having discussion with some SIG contributors, it looks like none of them are providing any way to make this feature available in auto instrumentation but only in manual instrumentation using SDK.

@sanketmehta28
Copy link
Member

It looks like Java agent is able to collect container.id out of the box (I have updated my last comment accordingly).
They load all ResourceDetectors available and collect resources from it (just like all instrumentors are being loaded in sizecustomize.py). This way container.id is available in span resource if it is available otherwise it will fail silently.
I guess till we finalize the ENV variable approach, we can use this approach to make this feature available for auto instrumentation.
Thoughts?

@sanketmehta28
Copy link
Member

I have checked with @svrnm and came to know that in java agent, they basically load all ResourceDetectors installed and get all resources from them. this way they get the container.id in the span resources if ContainerResourceDetector module is installed else fails silently.
I guess we can use the same approach to implement this feature in otel python agent
Thoughts anyone @open-telemetry/opentelemetry-python-contrib-approvers ?

@srikanthccv
Copy link
Member

else fails silently.

What is it that you refer to here that fails silently?

this way they get the container.id in the span resources

Whatever we do, It will not be limited just to the traces. The same aggregated resource must be used for all three "signals". I am okay with the idea of using all installed resource detectors, but I think the user should have the option to suppress it if they want to.

@sanketmehta28
Copy link
Member

What is it that you refer to here that fails silently?

Means it will not fetch the container.id without any error or warning and continue the execution

Whatever we do, It will not be limited just to the traces. The same aggregated resource must be used for all three "signals". I am okay with the idea of using all installed resource detectors, but I think the user should have the option to suppress it if they want to.

@srikanthccv : How do you suggest we provide the option to suppress it?

@svrnm
Copy link
Member Author

svrnm commented May 22, 2023

yeah 🎉

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

Successfully merging a pull request may close this issue.

4 participants