New api/label package, common label set impl
#651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmacd
requested review from
krnowak,
lizthegrey,
MrAlias,
paivagustavo,
rghetia and
tedsuo
as code owners
krnowak
suggested changes
Apr 21, 2020
There was a problem hiding this comment.
I like it. Maybe some general problems:
- other SDK's will be forced to use api's way of generating a unique representation of a label set
- creating resources with duplicates used to use the first-wins mechanics, now it's last-wins.
- we probably could drop label encoder use in more places and limit that to exporters only?
MrAlias
reviewed
Apr 21, 2020
|
PTAL. Note that this includes as a prerequisite the changes from #654 |
MrAlias
approved these changes
Apr 22, 2020
|
(I added more tests.) |
krnowak
approved these changes
Apr 23, 2020
There was a problem hiding this comment.
This looks great!
But I see that you have left the resource mostly as is, including the api dependency on sdk resource. I suppose this is something to be cleaned up in a future PR then.
What I found are mostly documentation issues, thus I'm going to approve the PR, so you can merge the PR right after fixing those.
api/label/set_test.go
Outdated
Comment on lines
95
to
104
| // No other String values are equal to this. | ||
| for _, distinct := range s2d { | ||
| if distinct[0] == d { | ||
| continue | ||
| } | ||
| for _, otherDistinct := range distinct { | ||
| require.NotEqual(t, otherDistinct, d) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same confusion here, but with strings.
This was referenced Apr 23, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This extracts the
sdk/metriclabel set implementation, thesdk/export/metricdefault label encoder, and moves them intoapi/label. The new type is*label.Setsupports the same iterator interface, but with the removal of two interfacesLabelsandLabelStorageare no longer needed, since the API uses structs and pointers.The main support implemented by the SDK's label set was computing a distinct value for every unique set of labels, after de-duplication and last-value-wins semantics are applied. The internal map key used in the SDK is now exposed as
label.Distinct, which is a struct wrapping a non-exportedinterface{}, wrapping a variable-size array.This replaces partly replaces #640 and #641, in that it changes how uniqueness is exposed and resources are represented.
Note that the new
label.Settype replaces both the metric label set and the resource struct in this PR. In a future PR it should also replace thecorrelation.Map(TODO).The
label.Setobject manages a cache of encodings by label encoder. This support is expanded to >1 encoding per label set, so that multiple exporters can co-exist without harming performance. The number of encodings is limited to 3 in this PR, but can be expanded in the future.