Document and check resource comparability #6272
Conversation
Ensure backwards compatibility by adding a compile-time check that the Resource remains comparable. Document the shortcomings of direct comparison of a Resource.
MrAlias
requested review from
XSAM,
dashpole,
pellared and
dmathieu
as code owners
sdk/resource/resource.go
Outdated
| // While Resource is comparable, its comparison should not be relied on when | ||
| // checking equality. The [Resource.Equal] method should be used to check | ||
| // equality between two resources and the [attribute.Distinct] returned from | ||
| // [Resource.Equivalent] should be used for map keys instead. No guantee of the |
There was a problem hiding this comment.
Can we actually add this? Wouldn't it be breaking if resource comparisons stoped working?
There was a problem hiding this comment.
I'm not intending to say that comparison is going away, just that the result of that comparison is not guaranteed to be stable. Is that not what you are interpreting?
There was a problem hiding this comment.
Line 36 is being added to ensure the resource will always remain comparable FWIW.
There was a problem hiding this comment.
I think this is acceptable in Go ecosystem. Especially given that the type already has Equal method.
I would suggest also giving a similar comment to Equal. Example: https://pkg.go.dev/time#Time.Equal I mean adding something similar to
See the documentation on the Time type for the pitfalls of using == with Time values; most code should use Equal instead.
There was a problem hiding this comment.
I would suggest also giving a similar comment to
Equal. Example: https://pkg.go.dev/time#Time.Equal I mean adding something similar toSee the documentation on the Time type for the pitfalls of using == with Time values; most code should use Equal instead.
Updated, PTAL.
There was a problem hiding this comment.
LGTM. I am not resolving so that @dashpole can see the conversation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6272 +/- ##
=====================================
Coverage 81.8% 81.8%
=====================================
Files 283 283
Lines 24892 24892
=====================================
Hits 20381 20381
Misses 4107 4107
Partials 404 404 |
sdk/resource/resource.go
Outdated
| // While Resource is comparable, its comparison should not be relied on when | ||
| // checking equality. The [Resource.Equal] method should be used to check | ||
| // equality between two resources and the [attribute.Distinct] returned from | ||
| // [Resource.Equivalent] should be used for map keys instead. No guantee of the |
There was a problem hiding this comment.
I think this is acceptable in Go ecosystem. Especially given that the type already has Equal method.
I would suggest also giving a similar comment to Equal. Example: https://pkg.go.dev/time#Time.Equal I mean adding something similar to
See the documentation on the Time type for the pitfalls of using == with Time values; most code should use Equal instead.
Model it after time.Time
| // | ||
| // Note that the Go == operator compares not just the resource attributes but | ||
| // also all other internals of the Resource type. Therefore, Resource values | ||
| // should not be used as map or database keys. In general, the [Resource.Equal] |
There was a problem hiding this comment.
Given this assertion ("should not be used as..."), I am a bit confused about the goal of line 38. If the we are saying "don't use Resource as a map key" why are we explicitly requiring it to be usable as a map key on line 38?
I get that formal backward compatibility requires it but if this was never a reasonable thing to do, perhaps we can decide that it is not a practical backward compatibility requirement we are bound by?
Do we have a document that says comparability is a backward compatibility criteria? Or perhaps a document that says "what compiled in the past should compile in the future" as a catch-all guarantee?
There was a problem hiding this comment.
This is inspired by how the Go standard library handles such issues. See: https://pkg.go.dev/time#Time
I would say we do our best to follow https://go.dev/doc/go1compat which is kind of:
what compiled in the past should compile in the future
with some exceptions.
The Collector does have it documented here: https://github.com/open-telemetry/opentelemetry-collector/blob/main/VERSIONING.md#go-modules
Probably we should also document it in our https://github.com/open-telemetry/opentelemetry-go/blob/main/VERSIONING.md
There was a problem hiding this comment.
with some exceptions.
What are the exceptions? The doc you linked to says: "It is intended that programs written to the Go 1 specification will continue to compile and run correctly, unchanged, over the lifetime of that specification". The "continue to compile" phrase doesn't seem to leave much room for exceptions.
There was a problem hiding this comment.
Please read the doc further 😉
https://go.dev/doc/go1compat#expectations
The Collector team did explain it even in a more clear way: https://github.com/open-telemetry/opentelemetry-collector/blob/main/VERSIONING.md#general-go-api-considerations
PS. We did similar thing here https://pkg.go.dev/go.opentelemetry.io/otel/attribute#Set
There was a problem hiding this comment.
OK. Can I argue for one more exception? That maintaining accidental comparability of the structs is not something we care about, unless it is explicitly documented (like it is done for attribute.Set)?
In this particular case it seems to create a whole lot of work to keep the Resource comparable, but unclear there is anyone to benefit from this work.
To be clear, I am not advocating for breaking a compatibility promise. I am merely suggesting that what previously was not explicitly promised is added as another exception (since we don't mind having exceptions).
Ensure backwards compatibility by adding a compile-time check that the Resource remains comparable.
Document the shortcomings of direct comparison of a Resource.