-
Notifications
You must be signed in to change notification settings - Fork 10.3k
labels: improve symmetry with stringlabels builds #12230
Description
Proposal
Hi!
I recently went through the effort to better align our usage of labels.Labels so that we can try out stringlabels builds and evaluate the change on tsdb, ingester, and querier components for an internal system we have.
There were a couple sharp edges I ran into trying to make things compatible with both stringlabels and !stringlabels builds that I wanted to bring to folks attention (I suppose mainly @bboreham ?) if they were not aware of them already.
-
There is no
Overwritemethod onlabels.ScratchBuilderfor!stringlabelsbuilds. I ended up having to write some intermediate struct for the time being to ensure we can do the equivalent for both build types. I noticed this isn't used anywhere in Prometheus, but was in Store series labels as a single string, to save memory grafana/mimir#3555 (I have similar needs with how it's being used in that PR). -
Both
InternStringsandReleaseStringsbehave unexpectedly withstringlabelsbuilds. Interning the internal representation of the labels here doesn't seem to make much sense (at least I can't think of a reason) compared to interning individual strings with the!stringlabelsbuild.I know there is some prototyping going on about potentially using symbol tables as a form of interning for
stringlabelsbuilds. Depending on how that pans out I am wondering if it even makes sense to keep these two methods around?In places we've been doing interning already we now have separate build files so that it's only done when
!stringlabels. -
I am kind of scared to call the
Lenmethod with astringlabelsbuild since it requires fully iterating and decoding the labels. If I need to properly size a map or slice before callingRangethe penality seems too high.Since the label names and values are RLE maybe we can do the same for the total number of labels?
-
Calls to methods like
GetandRangemight not return stable strings with astringlabelsbuild.There are certain code paths that heavily rely on the use of a single
labels.Labelsand calls tolabels.ScratchLabels.Overwriteto minimize allocations. With a!stringlabelsbuild the consuming end of the labels can safely retain references to strings returned byGet(assuming the labels were constructed with strings that won't mutate) where this is not true with astringlabelsbuild since a string slice of the underlying data structure is returned.This one was really tricky and difficult to reason about and for the time being I'm being very careful in defensively copying any string returned by labels methods that need to be retained.
I understand a string isn't defensively copied for calls to things like
GetandRangefor performance reasons, but this was surprising behavior for sure. I am hoping that maybe this problem will go away if the use of symbol tables works out :)
I'm more than happy to file PRs to address any discrepancies, but I'm also not really sure what direction things are headed and maybe some of these issues will naturally go away with the on-going work @bboreham is doing.