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

Return merged Resource on schema conflict #4876

Merged
merged 12 commits into from
Feb 5, 2024

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jan 31, 2024

Resolve #2341

Why not just log?

By returning a partial result (the merged conflicting resource), the user is able to decide what they want to do. If we just logged this as a warning, the user may unknowingly use a Resource they would rather have dropped or fixed.

Specification compliance

The specification states:

The resulting resource will have the Schema URL calculated as follows:

  • If the old resource's Schema URL is empty then the resulting resource's Schema URL will be set to the Schema URL of the updating resource,
  • Else if the updating resource's Schema URL is empty then the resulting resource's Schema URL will be set to the Schema URL of the old resource,
  • Else if the Schema URLs of the old and updating resources are the same then that will be the Schema URL of the resulting resource,
  • Else this is a merging error (this is the case when the Schema URL of the old and updating resources are not empty and are different). The resulting resource is undefined, and its contents are implementation-specific.

Based on this language we are given a fair amount of freedom to handle this conflict in the way we see fit. Looking at other language implementations, Go is the only one that will actually return an error and does not make the merge:

Language Behavior
Java logs conflict and returns merged
Javascript resources do not have schema URL
Python logs conflict and returns unmerged
Rust ignore conflict
Ruby resources do not have schema URL
PHP logs conflict and returns merged
.NET resources do not have a schema URL
C++ ignored conflict

Given the specification also states:

The resulting resource MUST have all attributes that are on any of the two input resources. 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).

Trying to resolve this conflict by actually using the schemas to make a transformation is not compliant. Additionally, given the reluctance to stabilize these transformations, it does not seem prudent to not provide a better user story here.

@MrAlias MrAlias added the area:resources Part of OpenTelemetry resources label Jan 31, 2024
@MrAlias MrAlias added this to the v1.23.0 milestone Jan 31, 2024
sdk/resource/resource.go Outdated Show resolved Hide resolved
@pellared

This comment was marked as outdated.

@MrAlias MrAlias force-pushed the rm-res-merge-conflic-err branch from 8ea4ac3 to ba391b8 Compare January 31, 2024 21:59
@MrAlias MrAlias changed the title Log error on resource schema conflict and returned merged Return merged Resource on schema conflict Jan 31, 2024
@MrAlias MrAlias force-pushed the rm-res-merge-conflic-err branch from ba391b8 to b2905b3 Compare January 31, 2024 22:03
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f4e1e04) 82.6% compared to head (cd62d36) 82.6%.

❗ Current head cd62d36 differs from pull request most recent head 2f74074. Consider uploading reports for the commit 2f74074 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4876   +/-   ##
=====================================
  Coverage   82.6%   82.6%           
=====================================
  Files        232     232           
  Lines      18862   18868    +6     
=====================================
+ Hits       15595   15601    +6     
  Misses      2977    2977           
  Partials     290     290           
Files Coverage Δ
sdk/resource/auto.go 91.6% <100.0%> (+0.9%) ⬆️
sdk/resource/resource.go 76.6% <100.0%> (+0.1%) ⬆️

@MrAlias MrAlias force-pushed the rm-res-merge-conflic-err branch 3 times, most recently from 0eaf6cd to 1aab4d0 Compare January 31, 2024 22:17
@MrAlias MrAlias force-pushed the rm-res-merge-conflic-err branch from 1aab4d0 to 0239f22 Compare January 31, 2024 22:23
@MrAlias MrAlias marked this pull request as ready for review January 31, 2024 22:29
sdk/resource/resource.go Outdated Show resolved Hide resolved
sdk/resource/resource.go Show resolved Hide resolved
sdk/resource/resource_test.go Show resolved Hide resolved
sdk/resource/resource.go Show resolved Hide resolved
sdk/resource/resource.go Outdated Show resolved Hide resolved
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

LGTM.

#4876 (comment) can be addressed as a follow-up. I can work on it on Tuesday.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I think that detect should be changed to handle conflicts better.

#4876 (comment)

@MrAlias MrAlias force-pushed the rm-res-merge-conflic-err branch from 4f1da09 to 9f117ab Compare February 1, 2024 16:15
sdk/resource/auto_test.go Outdated Show resolved Hide resolved
sdk/resource/auto_test.go Show resolved Hide resolved
@MrAlias MrAlias force-pushed the rm-res-merge-conflic-err branch from 1d6c1ae to b062513 Compare February 1, 2024 17:57
@MrAlias MrAlias merged commit eabcef4 into open-telemetry:main Feb 5, 2024
23 checks passed
@MrAlias MrAlias deleted the rm-res-merge-conflic-err branch February 5, 2024 15:33
@jufemaiz
Copy link

jufemaiz commented Feb 5, 2024

Big step forward! Thanks for the work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:resources Part of OpenTelemetry resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource merge semantics make upgrades problematic
7 participants