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.

Sign up for GitHub

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

Jump to bottom

Update Resource merge key conflict precedence #1544

Merged
merged 9 commits into from
Jan 28, 2021
Merged

Update Resource merge key conflict precedence #1544

merged 9 commits into from
Jan 28, 2021

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Jan 22, 2021 •
edited
Loading

Description

Fixes #1543

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • tox -e test-core-sdk

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

@srikanthccv srikanthccv requested review from a team, owais and codeboten and removed request for a team January 22, 2021 05:05
aabmass
aabmass requested changes Jan 22, 2021
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice! This was confusing in the spec before.

I think this line needs to be changed to keep the docstring on OTEL_RESOURCE_ATTRIBUTES accurate (OTEL_RESOURCE_ATTRIBUTES envvar should have lower precendence):

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py

Line 113 in 6489bf5

resource = _DEFAULT_RESOURCE.merge(Resource(attributes))

opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py Show resolved Hide resolved
@srikanthccv
Copy link
Member Author

@aabmass Thanks for pointing that out, I have added test to ensure the priority. Also updated the docstrings. Please take another look.

aabmass
aabmass approved these changes Jan 28, 2021
View reviewed changes
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.

Nice thanks!

@codeboten codeboten merged commit 18761d7 into open-telemetry:master Jan 28, 2021
@srikanthccv srikanthccv deleted the resource-merge branch September 24, 2021 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@aabmass aabmass aabmass approved these changes

@codeboten codeboten codeboten approved these changes

@owais owais Awaiting requested review from owais

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update Resource merge method precedence for conflict key and empty string
4 participants
@srikanthccv @codeboten @aabmass @lzchen