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

Difficulty using Resource::from_detectors to set service.name correctly #1298

Open
BrynCooke opened this issue Oct 13, 2023 · 3 comments
Open
Milestone

Comments

@BrynCooke
Copy link
Contributor

BrynCooke commented Oct 13, 2023

SdkProvidedResource detector currently has to know about all other detectors to correctly set service.name.

Detectors currently fill out resources in a last one wins fashion. In the following situation EnvResourceDetector, upon finding service.name will overwrite the value even if the SdkProvidedResourceDetector obtained it from OTEL_SERVICE_NAME.

let resource = Resource::from_detectors(
            time::Duration::from_secs(5),
            vec![
                Box::new(SdkProvidedResourceDetector),
                Box::new(EnvResourceDetector::new()),
            ],
        );

It's possible to reverse to ordering of the resource detectors to get the right result, but this doesn't support adding other resource detectors.

let resource = Resource::from_detectors(
            time::Duration::from_secs(5),
            vec![
                Box::new(MyResourceDetector::new()),
                Box::new(EnvResourceDetector::new()),
                Box::new(SdkProvidedResourceDetector),
            ],
        );

In this case SdkProvidedResourceDetector will override any value obtained by MyResourceDetector with unknown_service if it can't find anything from env.

Relates to #1295

@BrynCooke
Copy link
Contributor Author

BrynCooke commented Oct 13, 2023

The issue seems to be that if a detector does defaulting then it leads to issues. The following works:

 let resource = Resource::from_detectors(
            time::Duration::from_secs(5),
            vec![
                Box::new(MyResourceDetector::new()),
                Box::new(EnvResourceDetector::new()),
                Box::new(EnvServiceNameDetector::new()), //Only sets service.name if it exists in env.
            ],
        );
        

Proposal: If instead Resouce itself is responsible for defaulting service name then it can take a look at the attributes after all detectors have been resolved and see if service name is missing.

@hdost hdost added this to the Tracing SDK Stable milestone Oct 25, 2023
@flokli
Copy link

flokli commented Jan 3, 2024

I'm interested in this as well. I initially made an attempt extending SdkProvidedResourceDetector with a fallback_service_name: Option<String> (deriving Default) and adding a constructor for the fallback name, but it requires everyone to "properly" create an instance (you can't just use SdkProvidedResourceDetector.detect anymore).

I think your approach of letting Resource take care of defaulting a service name if unset is nicer, and I favor it.

@cijothomas cijothomas modified the milestones: Tracing SDK Stable, 0.28.0 Dec 11, 2024
@cijothomas
Copy link
Member

@pitoniak32 Is this something you can help triage, as you already helping in this area?

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

No branches or pull requests

4 participants