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

Populate resource attributes as per semantic conventions #1053

Merged
merged 13 commits into from
Sep 3, 2020

Conversation

codeboten
Copy link
Contributor

As per semantic conventions , this changes sets the telemetry.sdk resource attributes.

Fixes #1033

Type of change

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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • ran unit tests

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been updated

As per semantic conventions, set the `telemetry.sdk` parameters.
@codeboten codeboten requested a review from a team August 28, 2020 05:53
@codeboten codeboten changed the title semantic conventions: populate resource attributes Populate resource attributes as per semantic conventions Aug 28, 2020
@lzchen
Copy link
Contributor

lzchen commented Aug 31, 2020

Was it established that we were not going the resource detector route? @aabmass

@codeboten
Copy link
Contributor Author

Was it established that we were not going the resource detector route? @aabmass

That was my understanding based on the last comment here: #932 (comment)

@lzchen
Copy link
Contributor

lzchen commented Aug 31, 2020

Was it established that we were not going the resource detector route? @aabmass

That was my understanding based on the last comment here: #932 (comment)

From the comment, it is not exactly clear to me what was the decision. I'm not sure "He suggested yesterday doing it right in the providers." refers to. Doing what in the providers? Instantiating Resources with defaults or instantiating a default resource provider somewhere else and populating the resources within the providers?

@codeboten
Copy link
Contributor Author

Was it established that we were not going the resource detector route? @aabmass

That was my understanding based on the last comment here: #932 (comment)

From the comment, it is not exactly clear to me what was the decision. I'm not sure "He suggested yesterday doing it right in the providers." refers to. Doing what in the providers? Instantiating Resources with defaults or instantiating a default resource provider somewhere else and populating the resources within the providers?

Ah I see what you mean. I took it as instantiating the provider with the default resource which would contain the sdk attributes.

@aabmass
Copy link
Member

aabmass commented Sep 2, 2020

Talked with James yesterday and he thinks this solution is fine. The semantic convention says:

The default OpenTelemetry SDK provided by the OpenTelemetry project MUST set telemetry.sdk.name to the value opentelemetry.

And I think the only way to accomplish this is adding the detection in the providers, not requiring the user to do it. The only other special case that I know of (that must be done in the provider) is this:

The SDK MUST extract information from the OTEL_RESOURCE_ATTRIBUTES environment variable and merge this, as the secondary resource, with any resource information provided by the user, i.e. the user provided resource information has higher priority.

These could be done by implementing ResouceDetector, but I take it that we don't rely on the user to merge these labels in, we do it automatically (in providers). Here's what I found for OT Java's implementation


The other resources/semantic conventions that I know of are not worded as strongly (e.g. container). For these others, we can provide resource detector(s) for users to run, though I don't think it is required for GA?

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.

LGTM

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM. I think #1063 would be a good followup for this.

@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Sep 3, 2020
@codeboten codeboten merged commit 8992741 into open-telemetry:master Sep 3, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga To be resolved before GA release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK should populate telemetry.sdk.* resource attributes
3 participants