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

Make Resource.Merge consistent with Span.SetAttribute. #1345

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Jan 15, 2021

Required for #1346.
Resolves parts of #1184.

As agreed (except for the empty-string change) in the spec meeting, this is a pre-requesite for the updated default resource wording of #1294.

Changes

  • Remove special case for empty string when merging resources.
  • Let the second argument take precedence, i.e. x.Merge(y) will have values from y on conflict.

@Oberon00 Oberon00 force-pushed the resource-merge-reverse branch from 950f8c2 to 39fafa5 Compare January 15, 2021 12:11
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Yes please!

@carlosalberto
Copy link
Contributor

LGTM

@Oberon00
Copy link
Member Author

Actually, with the new behavior, I would like to change the specified name from Merge to UpdatedWith. If you agree, please upvote this comment and I can change the PR.

@anuraaga
Copy link
Contributor

Preference for Merge over UpdatedWith since it's shorter.

For some reference, JS has the reasonably short Object.assign which is similar to the proposed behavior.

@@ -43,27 +43,21 @@ Required parameters:

### Merge

The interface MUST provide a way for a primary resource and a
secondary resource to be merged into a new resource.
The interface MUST provide a way for an old resource and an
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The interface MUST provide a way for an old resource and an
The interface MUST provide a way for an existing resource and an

Copy link
Member Author

@Oberon00 Oberon00 Jan 15, 2021

Choose a reason for hiding this comment

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

The other resource also exists. Maybe "updated", and rename "updating" to "new"? Or "previous" and "new".

Copy link
Contributor

Choose a reason for hiding this comment

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

"old" makes it sound like it's.... old, which is might not be at all.

Copy link
Member Author

@Oberon00 Oberon00 Jan 15, 2021

Choose a reason for hiding this comment

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

I named it "old" to emphasize that the values in there may be overwritten by the other resource. But I agree that the name is a bit odd. Maybe we could specify it as a member function resource.Merge(updating). Then we don't have to come up with a name for the left-hand-side.

EDIT: If I get some more upvotes from approvers on this comment, I'll change the PR (assuming this is an editorial-only change)


Attribute key namespacing SHOULD be used to prevent collisions across different
resource detection steps.
If a key exists on both the old and updating resource, the value of the updating
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If a key exists on both the old and updating resource, the value of the updating
If a key exists on both the existing and updating resource, the value of the updating

Attribute key namespacing SHOULD be used to prevent collisions across different
resource detection steps.
If a key exists on both the old and updating resource, the value of the updating
resource MUST be picked (even if the updated value is empty).
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

About the behavior for empty strings? I hope so, it is one special case we lose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @bogdandrutu ;)

@carlosalberto
Copy link
Contributor

@Oberon00 Please rebase your branch ;)

@Oberon00
Copy link
Member Author

@carlosalberto Done.

@carlosalberto carlosalberto merged commit 77bfc63 into open-telemetry:master Jan 20, 2021
@Oberon00 Oberon00 deleted the resource-merge-reverse branch January 20, 2021 17:08
evantorrie added a commit to evantorrie/opentelemetry-go that referenced this pull request Jan 29, 2021
MrAlias added a commit to open-telemetry/opentelemetry-go that referenced this pull request Jan 29, 2021
* Reverse order of attribute precedence when merging two Resources

This reflects a change in the specification itself.
open-telemetry/opentelemetry-specification#1345

* Resolves #1500

Co-authored-by: Tyler Yahn <[email protected]>
ldelossa pushed a commit to ldelossa/opentelemetry-go that referenced this pull request Mar 5, 2021
…n-telemetry#1501)

* Reverse order of attribute precedence when merging two Resources

This reflects a change in the specification itself.
open-telemetry/opentelemetry-specification#1345

* Resolves open-telemetry#1500

Co-authored-by: Tyler Yahn <[email protected]>
shbieng added a commit to shbieng/opentelemetry-go that referenced this pull request Aug 26, 2022
* Reverse order of attribute precedence when merging two Resources

This reflects a change in the specification itself.
open-telemetry/opentelemetry-specification#1345

* Resolves #1500

Co-authored-by: Tyler Yahn <[email protected]>
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:resource Related to the specification/resource directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants