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

Add default and service name to get_aggregated_resource #2013

Merged
merged 3 commits into from
Aug 13, 2021

Conversation

NathanielRN
Copy link
Contributor

@NathanielRN NathanielRN commented Jul 30, 2021

Description

We want the business metrics _DEFAULT_RESOURCE attributes to be set on the resource returned by get_aggregated_resources(), and we also want the service_name to be set to some default if it is not.

In this PR, we update get_aggregated_resources() so that it adds both those things, and the Resource.create() will call it instead so that it retains all its attributes as before.

The get_aggregated_resources() method had something added to its API, but other than that nothing API related changes.

Fixes #1996

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

N/A

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
    - [ ] Documentation has been updated

@NathanielRN NathanielRN marked this pull request as ready for review July 30, 2021 23:18
@NathanielRN NathanielRN requested a review from a team as a code owner July 30, 2021 23:18
@NathanielRN NathanielRN requested review from codeboten and aabmass and removed request for a team July 30, 2021 23:18
)
return resource

return get_aggregated_resources(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think simply adding _DEFAULT_RESOURCE to OTELResourceDetector is a more elegant solution. It's also kind of weird that we are forcing create to have the same codepath as get_aggregated_resources, since the latter takes in detectors but the former has nothing to do with detectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reply

If we do that though, then the service.name won't default to unknown_service:test as is required in the specs. If users simply use get_aggregated_resources alone, they would have to do

my_attributes_resource = Resource.create(my_attributes)
my_aggregated_resource = get_aggregated_resources([MyResourceDetector])
my_resource = my_aggregated_resource.merge(my_attributes_resource)

for example, in which case OTELResourceDetectorwill get called twice by get_aggregated_resources and Resource.create().

We cannot use initial_resource= as I explained this comment on the PR because subsequent detectors will overwrite attributes from the original my_attributes.

It's also kind of weird that we are forcing create to have the same codepath as get_aggregated_resources, since the latter takes in detectors but the former has nothing to do with detectors.

I think this is okay... Resource.create() is already using OTELResourceDetector right? So it's not entirely independent of "detectors" as it is right now.

if not attributes:
attributes = {}
resource = _DEFAULT_RESOURCE.merge(
OTELResourceDetector().detect()
).merge(Resource(attributes, schema_url))
if not resource.attributes.get(SERVICE_NAME, None):

I think having it return get_aggregated_resources([]) just rewords what it is already doing in a more clear manner. Except now we move the defaults users should want (opentelemetry-python business metrics) into get_aggregated_resources so that they only have to use one function, not 2. (Resource.create() AND get_aggregated_resources).

Action Items

Looking at it more, I think get_aggregated_resources and Resource.create() have overlapping responsibilities. Because they both use OTELResourceDetectors, but only the former should need it.

I think the right answer is to deprecate the Resource.create(my_attributes) method, and users should only ever use

  • Resource() (with no defaults) or
  • get_aggregated_resources([]) which would do the same as Resource.create() (defaults + service.name) right now or
  • get_aggregated_resources([detector1, detector2]) which starts with (defaults + service.name), and adds detectors afterwards
  • (OPTIONAL) get_aggregated_resources([detector1, detector2], final_attributes=my_attributes) which starts with (defaults + service.name), adds detectors afterwards, and finally adds my attributes with the highest priority

Copy link
Contributor

@lzchen lzchen Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NathanielRN

We shouldn't change anything about Resource.create(), this is what we recommend to users to do to create a Resource. create is also the thing that is speced out, get_aggregated_resources() is a function we made for convenience.
I think your change for get_aggregated_resource() is actually fine, but instead of having the logic in that, just leave it in Resource.create() and call create() from within get_aggregated_resource(). Essentially the same change as yours but the reverse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lzchen ! That makes a lot of sense 🙂 .

I updated my PR to do what you said and the tests passed locally so I'll go ahead with that as the solution!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding onto that, get_aggregated_resources() is only a convenience to run multiple detectors in parallel and merge them in order. OTelResourceDetector is also just an implementation detail of how we pull in from the envvars. It is exposed publicly which was maybe a mistake, but should be considered an implementation detail of Resource.create().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding onto that, get_aggregated_resources() is only a convenience to run multiple detectors in parallel and merge them in order.

Hm, so the way you read the method name get_aggregated_resources(detectors), would it surprise you for it to try to set _DEFAULT_RESOURCE or service.name in the attributes? Because that is what this PR is adding to it.

I can see it going either way as to whether they should be added from a developer's POV.

But as a user, I think I am more likely to forget that I need to do

Resource.create().merge(get_aggregated_resources(detectors)

and would just do

get_aggregated_resources(detectors)

which would make me miss out on attributes set by Resource.create(). Then, as a user, I would want get_aggregated_resources(detectors) to just handle that for me and set the necessary defaults.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so the way you read the method name get_aggregated_resources(detectors), would it surprise you for it to try to set _DEFAULT_RESOURCE or service.name in the attributes? Because that is what this PR is adding to it.

No, it doesn't surprise me. I agree this PR fixes unexpected behavior. I'm just agreeing with Leighton that get_aggregated_resources([]) should NOT be a pattern we encourage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I didn't realize that Resource.create() was a spec defined function, so it makes sense that get_aggregated_resources should call it and not the other way around 🙂

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@open-telemetry/python-maintainers are we all OK this behavior change going out as a minor-release bugfix?

@NathanielRN NathanielRN closed this Aug 6, 2021
@NathanielRN NathanielRN reopened this Aug 6, 2021
@NathanielRN NathanielRN mentioned this pull request Aug 6, 2021
@owais
Copy link
Contributor

owais commented Aug 12, 2021

@NathanielRN can you please force push this to trigger CI again?

Comment on lines +8 to +9
- Fix documentation on well known exporters and variable OTEL_TRACES_EXPORTER which were misnamed
([#2023](https://github.com/open-telemetry/opentelemetry-python/pull/2023))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing the formatting on this while I am passing by

@lzchen lzchen closed this Aug 13, 2021
@lzchen lzchen reopened this Aug 13, 2021
@ocelotl ocelotl merged commit 3cdb54f into open-telemetry:main Aug 13, 2021
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

Successfully merging this pull request may close these issues.

Resource aggregator method is missing Default Resource Bizmetric Attributes
6 participants