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 aggregator method is missing Default Resource Bizmetric Attributes #1996

Closed
NathanielRN opened this issue Jul 27, 2021 · 8 comments · Fixed by #2013
Closed

Resource aggregator method is missing Default Resource Bizmetric Attributes #1996

NathanielRN opened this issue Jul 27, 2021 · 8 comments · Fixed by #2013
Labels
bug Something isn't working

Comments

@NathanielRN
Copy link
Contributor

NathanielRN commented Jul 27, 2021

Describe your environment

As of OTel Python 1.4

Steps to reproduce

If you want to use custom ResourceDetectors, we recommend you use the get_aggregated_resources method because it will automatically merge your custom ResourceDetector with the OTELResourceDetector (used to read ResourceDetector attributes from the OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables).

However, if you do that, you miss out on the "business metrics" attributes that get created with Resource.create()

_DEFAULT_RESOURCE = Resource(
{
TELEMETRY_SDK_LANGUAGE: "python",
TELEMETRY_SDK_NAME: "opentelemetry",
TELEMETRY_SDK_VERSION: _OPENTELEMETRY_SDK_VERSION,
}
)

Resource.create() actually uses OTELResourceDetector too. So if you try to get the bizmetrics by doing get_aggregated_resources([ MyCustomResourceDetector(), Resource.create() ]), you will get OTELResourceDetector twice. (Once from Resource.create() and once from get_aggregated_resources. See the code here:

resource = _DEFAULT_RESOURCE.merge(
OTELResourceDetector().detect()
).merge(Resource(attributes, schema_url))

detectors = [OTELResourceDetector()] + detectors

What is the expected behavior?
Bizmetrics should be on every span which uses the aggregated Resource from get_aggregated_resources

What is the actual behavior?
Using get_aggregated_resources as we recommend causes bizmetrics to be missed.

Additional context

The most obvious solutions would be to

  • (Option 1) Remove OTELResourceDetector from get_aggregated_resources's default implementation so users can add either OTELResourceDetector or Resource.create() depending on what they want
  • (Option 2) Replace OTELResourceDetector in get_aggregated_resources with Resource.create() so spans always get bizmetrics by default

Since we already 1.0+, I recommend Option 2. This will just "Add" attributes and won't break anyone using this method who expects OTELResourceDetector attributes to be there when they use get_aggregated_resources. They will just get bizmetric attributes for free.

@NathanielRN NathanielRN added the bug Something isn't working label Jul 27, 2021
@NathanielRN
Copy link
Contributor Author

Option 2 would require a one line fix:

    detectors = [OTELResourceDetector()] + detectors

becomes:

    detectors = [Resource.create()] + detectors

@aabmass
Copy link
Member

aabmass commented Jul 28, 2021

  • (Option 1) Remove OTELResourceDetector from get_aggregated_resources's default implementation so users can add either OTELResourceDetector or Resource.create() depending on what they want

This can already be done by passing Resource.create() here, right?

initial_resource: typing.Optional[Resource] = None,

For Option 2, you could just change the default for initial_resource to be _DEFAULT_RESOURCE?

@NathanielRN
Copy link
Contributor Author

This can already be done by passing Resource.create() here, right?

Yes but it will duplicated adding OTELResourceDetector().

For Option 2, you could just change the default for initial_resource to be _DEFAULT_RESOURCE?

Yes that's a good point! But we would miss out of the other stuff that Resource.create() does automatically (like setting the service name to unknown_service if no service name is provided and which is required by the spec):

if not resource.attributes.get(SERVICE_NAME, None):
default_service_name = "unknown_service"
process_executable_name = resource.attributes.get(
PROCESS_EXECUTABLE_NAME, None
)
if process_executable_name:
default_service_name += ":" + process_executable_name
resource = resource.merge(
Resource({SERVICE_NAME: default_service_name}, schema_url)
)
return resource

Also I don't think it would be an issue, but if we every treated initial_resource as a mutable argument then multiple calls to this method get_aggregated_resource would modify the same object right? It's that Python gotcha with parameter initializers. (The example uses a list but I think it applies to this object of type Resource too?). Again, shouldn't be an issue because we don't modify it, but it's probably safer to leave it as None?

@lzchen
Copy link
Contributor

lzchen commented Jul 30, 2021

I don't think detectors = [Resource.create()] + detectors would work because Resource.create() doesn't return a detector.

Can we just include _DEFAULT_RESOURCE in OTELResourceDetector and then modify Resource.create() to only use OTELResourceDetector by default?

@NathanielRN
Copy link
Contributor Author

@lzchen That's a great call out!

I came up with a similar solution in #2013, except I didn't add the _DEFAULT_RESOURCE to OTELResourceDetector. I thought as a detector it should only "detect" things like it does from the OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables.

But I would be okay with it also having the default attributes!

@aabmass
Copy link
Member

aabmass commented Aug 4, 2021

However, if you do that, you miss out on the "business metrics"

Unrelated, where did you see these called "business metrics?" I've never heard that lol

@aabmass
Copy link
Member

aabmass commented Aug 4, 2021

Yes that's a good point! But we would miss out of the other stuff that Resource.create() does automatically (like setting the service name to unknown_service if no service name is provided and which is required by the spec):

Oh that's weird. I'm actually confused why all of that is happening in Resource.create(). Since the purpose of _DEFAULT_RESOURCE is to cache the defaults, I would have expected all of that to be in _DEFAULT_RESOURCE and Resource.create() to just do _DEFAULT_RESOURCE.merge(Resource(attributes, schema_url))

@NathanielRN
Copy link
Contributor Author

Unrelated, where did you see these called "business metrics?" I've never heard that lol

Lol I hadn't heard about this either 😅 My manager was using this term a lot and it wasn't until I finally asked that I found out she was referring to "attributes which identify where the telemetry is coming from and which helps the business people use metrics to justify how many users are using this telemetry producer" 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants