PROM-39: implement type and unit labels for PRW 2.0 #16774
Conversation
storage/remote/write_handler.go
Outdated
| Type: modelMd.Type, | ||
| Unit: modelMd.Unit, | ||
| } | ||
| m.SetToLabels(builder) |
There was a problem hiding this comment.
I tried to find a way to add these to the ScratchBuilder we create above, but the ts.ToLabels function resets and then returns the completed labels. Let me know if making a new builder to add these two labels is likely to be too expensive, and I can try to explore modifying ToLabels, or adding a new function somewhere to desymbolize and also add type and unit labels.
There was a problem hiding this comment.
I'm not sure what the overhead cost is of creating the second builder is, but probably enough that we want to avoid doing it on every iteration when this feature flag is set.
Modifying ToLabels in write/v2/codec.go seems reasonable, or adding a ToLabelsWithMetadata there as well.
There was a problem hiding this comment.
Absolutely change the codec, it's ours and it can be expanded 👍🏽
There was a problem hiding this comment.
I was initially thinking about NOT reseting and renaming our methods to AddToLabels etc, but there's a challenge -- we need to ensure NO duplicates and priority over metadata values for type and unit labels.
Thus, I would just add desymbolizeLabelsWithMetadata priv func indeed that will do this and perhaps ToLabels(b, symbols, metaAware bool), ToLabelsWithMetadata or ToLabelsMetaAware(...) indeed. I would vote for the bool inside existing method so it's very clear for every new use of this PRW2 that you can make this decision of adding those new labels or not, otherwise it's less discoverable/known if ToLabels handles that or not
WDYT?
There was a problem hiding this comment.
I added "AddToLabels" to allow me to re-use the builder. Seems to align with other similar functions, and prevents the writev2 package from depending on the metadata package.
There was a problem hiding this comment.
@bwplotka can you clarify? Should user labels take precedence over labels from metadata? I can just change the ordering.
storage/remote/write_handler.go
Outdated
| Type: modelMd.Type, | ||
| Unit: modelMd.Unit, | ||
| } | ||
| m.SetToLabels(builder) |
There was a problem hiding this comment.
I was initially thinking about NOT reseting and renaming our methods to AddToLabels etc, but there's a challenge -- we need to ensure NO duplicates and priority over metadata values for type and unit labels.
Thus, I would just add desymbolizeLabelsWithMetadata priv func indeed that will do this and perhaps ToLabels(b, symbols, metaAware bool), ToLabelsWithMetadata or ToLabelsMetaAware(...) indeed. I would vote for the bool inside existing method so it's very clear for every new use of this PRW2 that you can make this decision of adding those new labels or not, otherwise it's less discoverable/known if ToLabels handles that or not
WDYT?
Signed-off-by: David Ashpole <dashpole@google.com>
Signed-off-by: David Ashpole <dashpole@google.com>
Signed-off-by: David Ashpole <dashpole@google.com>
Part of #16610
This implements the type and unit labels feature for the 2.0 handler.
cc @ArthurSens @bwplotka @aknuds1 @carrieedwards