internal/exp/metrics: identity types #31017
Conversation
Adds new internal, _experimental_ package `metrics/identity` which implements identity types for resource, scope, metric and stream.
| ) | ||
|
|
||
| type Resource struct { | ||
| attrs [16]byte |
There was a problem hiding this comment.
Do we need to include the resource SchemaURL as part of the identifier?
There was a problem hiding this comment.
no idea, can someone clarify? @jpkrohling @djaglowski
There was a problem hiding this comment.
There was a problem hiding this comment.
This came up in the maintainers meeting yesterday and consensus seemed to be that most language libraries are not setting this field. Technically it is optional so I would be ok with leaving it out for now and adding later if necessary.
| type Scope struct { | ||
| res Resource | ||
|
|
||
| name string |
There was a problem hiding this comment.
Do we need to include the scope schemaURL as part of the identifier?
|
Added some small comments. Overall, I like the approach a lot; the code design is very clean |
| sum.Write([]byte{byte(i.ty), mono, byte(i.temporality)}) | ||
| return sum | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we add functions for each identity struct to return the "inner" ID? For example:
func (i Metric) Scope() Scope {
return i.scope
}
So Stream has a Metric(), Metric has a Scope(), and Scope has a Resource().
There was a problem hiding this comment.
yes absolutely, already got to this conclusion during local testing
|
@jpkrohling @djaglowski Any thoughts on this PR? @sh0rez What do you think of my comments about the schemaURL and potentially allowing exposing the "inner" ID? Can i assist in any way to help get this PR across the finish line? (And subsequently, the follow-up PR for staleness: #31089) |
There was a problem hiding this comment.
I'm OK with this change, but it should be discussed with other folks owning metrics components. I also feel like this should be part of core, not contrib.
cc @dmitryax, @codeboten, @mx-psi
.github/CODEOWNERS
Outdated
| @@ -100,7 +100,7 @@ extension/storage/filestorage/ @open-telemetry/collect | |||
|
|
|||
| internal/aws/ @open-telemetry/collector-contrib-approvers @Aneurysm9 @mxiamxia | |||
| internal/docker/ @open-telemetry/collector-contrib-approvers @rmfitzpatrick @jamesmoessis | |||
|
|
|||
| internal/exp/metrics/ @open-telemetry/collector-contrib-approvers @sh0rez | |||
There was a problem hiding this comment.
@RichieSams, how do you feel about being a code owner of this with @sh0rez ?
There was a problem hiding this comment.
That's fine by me. I already was adding myself in the followup PR: #31089
There was a problem hiding this comment.
@RichieSams, you'll have to become a member of the OpenTelemetry org in order to be a code owner. Requirements are here. I'm happy to be one of your sponsors.
There was a problem hiding this comment.
that would apply to me as well then. @djaglowski would you be willing to be a sponsor for me as well?
There was a problem hiding this comment.
@jpkrohling Would you be willing to be our second sponsor?
There was a problem hiding this comment.
This looks pretty good to me. Having a dedicated package which handles uniqueness according to our data model will be very helpful. I've seen numerous other implementations, most of which tend to be suboptimal. As a shared package, it can evolve towards correctness and performance without otherwise being a concern of those who work on it.
I agree but in my opinion it makes sense to develop it in contrib and move it to core when it is proven out. |
|
@sh0rez It looks like some CI checks are failing: @djaglowski @jpkrohling @mx-psi Once that is fixed, do you have any other comments? Is it ok to merge? |
left by accident, no need here
|
@RichieSams as a future codeowner, could you approve this PR as well? :) |
Adds a new internal, _experimental_ package `metrics/identity` which implements identity types for resource, scope, metric and stream. This is closely related to work being done in open-telemetry#30707 and open-telemetry#30827. The package is specifically experimental, as it shall be treated as an internal component to above processors which may change at any moment as long as those are under active initial development. /cc @jpkrohling @djaglowski @RichieSams
Adds a new internal, experimental package
metrics/identitywhich implements identity types for resource, scope, metric and stream.This is closely related to work being done in #30707 and #30827.
The package is specifically experimental, as it shall be treated as an internal component to above processors which may change at any moment as long as those are under active initial development.
/cc @jpkrohling @djaglowski @RichieSams