Repository navigation
Conversation
this allows to query nhcb with _buckets, _sum or _count suffix and get classic histograms Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
Signed-off-by: r.bizos <r.bizos@criteo.com> Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
This allows to preserve ordering and thus having sorted output Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
fixing lint as well Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
fixing typo on feature flag name Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: rbizos <58781501+rbizos@users.noreply.github.com>
Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
Performance improvements - added a cache to not create the labels per samble but rather per serie - use hash rather than label.String in the converter - only convert _sum/_buckets/_count when needed (this significantly improved the _count/sum as buckets are the most expensive and were built each time) Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
the typo was fixed on the code but not in the docs Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
The NHCB-as-classic querier buffers the classic series it selected, but dropped the warnings of that series set on the floor. Signed-off-by: Bartek Plotka <bwplotka@google.com>
Adds --enable-feature=promql-classic-as-nhcb, the opposite direction of promql-nhcb-as-classic: a query for a native histogram also selects the classic histogram series of the same metric and folds them into an NHCB at query time. Classic histograms that cannot be converted are skipped per timestamp and reported as a warning annotation. Enabling both compatibility layers at once is rejected, as they would convert each other's output back and duplicate every histogram series. Signed-off-by: Bartek Plotka <bwplotka@google.com>
End-to-end tests for promql-nhcb-as-classic and promql-classic-as-nhcb that pin down what happens when both representations of a histogram are in storage: disjoint time ranges work, overlapping samples fail with a labelset collision, aggregations silently count both, and le matchers have to use the normalized bucket boundary. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…sion Pure refactor, so that the upcoming conversion of exponential native histograms to classic histogram series can reuse it. Signed-off-by: Bartek Plotka <bwplotka@google.com>
ConvertExponentialToClassic is the counterpart of ConvertNHCBToClassic for native histograms with an exponential schema. As exponential histograms have no fixed bucket boundaries, the caller passes the le boundaries of the _bucket series to emit. The value of each of them is the number of observations in the buckets whose upper boundary is at most le, which is exact for the bucket boundaries of the histogram and of any lower schema. AppendClassicBoundaries returns the boundaries representing a single histogram best: the upper boundaries of its buckets, plus the lower boundary of the lowest bucket and of the first bucket after a gap. The latter keep the linear interpolation of a classic histogram_quantile() within the buckets that hold observations, rather than spreading it across gaps or down to zero. To aggregate converted histograms by le, e.g. across series, callers can convert all of them with the union of their boundaries at the lowest schema amongst them. Signed-off-by: Bartek Plotka <bwplotka@google.com>
A classic histogram series converted from an NHCB used to stay queryable until the end of the lookback window when the NHCB went stale, or when the NHCB changed its bucket layout and did not have the bucket anymore. With a layout change, this mixed the old and the new buckets in histogram_quantile(). Mark a converted series stale at the first sample of its NHCB that does not result in it anymore, just like the scrape loop marks the series stale that disappear from a target. Signed-off-by: Bartek Plotka <bwplotka@google.com>
Stale markers of the classic series used to be converted like any other sample. When a classic histogram went stale, the NaN counts failed the conversion with a spurious "count mismatch: count=NaN" warning, and the last converted NHCB stayed queryable until the end of the lookback window. When a bucket was dropped from the exposition, the NHCB got NaN buckets. Skip the stale samples instead. If all the classic series of a histogram are stale at a timestamp, e.g. because the target went away, the NHCB is marked stale there. Otherwise the remaining series are converted, e.g. because the bucket layout changed. Signed-off-by: Bartek Plotka <bwplotka@google.com>
Pure refactor, so that the upcoming conversion of exponential native histograms can pick the classic bucket boundaries from all of them before converting the first one. The native histograms are read as float histograms, which the conversion supports just as well. Signed-off-by: Bartek Plotka <bwplotka@google.com>
Allow NHCBAsClassicQuerier to convert native histograms with an exponential schema, too, not only NHCB. This is not enabled for the promql-nhcb-as-classic feature flag, but for the upcoming flag making native and classic histograms interchangeable. Exponential histograms have no fixed bucket boundaries. So that the converted classic histograms can be aggregated by le, across series and over time, e.g. with sum by (le) (rate(...)), all of them are converted with the same le boundaries: the union of the boundaries of all the selected exponential histograms, reduced to the lowest schema amongst them. As the boundaries of a lower schema are boundaries of every higher schema, too, each conversion is exact. The price is that the le set depends on the selected series and time range. Staleness markers of histogram series are read back from the TSDB with schema 0, so they are explicitly excluded from the conversion. Signed-off-by: Bartek Plotka <bwplotka@google.com>
promql-nhcb-as-classic and promql-classic-as-nhcb convert in one direction each, and cannot be enabled together: stacking both layers converts the output of one layer back with the other one, returning every histogram series twice. Add the promql-nh-classic-compat feature flag, which makes native and classic histograms interchangeable in queries in both directions, e.g. while migrating from one to the other: * A query for classic histogram series (_bucket, _count or _sum) also returns the classic series converted from the native histograms of the base metric name. Unlike promql-nhcb-as-classic, this includes native histograms with an exponential schema, not only NHCB. * Any other query also returns the NHCB assembled from the classic histogram series of the queried metric name. NHClassicCompatQuerier dispatches every Select to exactly one of the two conversions, both of them wrapping the original querier, so converted series are never converted back. The new flag cannot be combined with either of the existing ones. The PromQL tests cover classic only, NHCB only and exponential only metrics, a partially migrated metric, exponential histograms with different buckets aggregated by le, a schema change, staleness in both directions, and the collisions of overlapping representations. Signed-off-by: Bartek Plotka <bwplotka@google.com>
Describe how to turn the three prototype feature flags into one feature: one flag with the representations to convert from, per selector control and debugging with __convert_stored_as__ and __debug_stored_as__ matchers, and stored data winning over converted data. The proposal lives next to the code for now. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…n package Move the NHCB as classic, classic as NHCB and combined queriers, their tests and the PromQL tests of the compatibility layer from the storage and promql packages to a new storage/histogramconv package, without behaviour changes. The package gets its own copies of the storage package's unexported sample types and test mocks. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…ram-conversion The three prototype feature flags are replaced with a single promql-histogram-conversion feature flag and a --query.convert-histograms-from flag, which lists the representations (classic, nhcb, nhe) to convert from. Nothing is converted by default. Conversion is now an engine option that wraps the querier of each query, instead of wrapping the local storage under the fanout. Data from remote read storage is converted too, and the remote read endpoint no longer returns converted series. Only selectors with a single equality metric name matcher are converted, regex name matchers are not anymore. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…s__ matchers
Matchers on __convert_stored_as__ select the representations (classic,
nhcb, nhe) a selector reads, stored or converted, and override
--query.convert-histograms-from for that selector. E.g.
foo_bucket{__convert_stored_as__="nhcb"} only returns the classic series
converted from NHCB, and foo{__convert_stored_as__="nhe"} only the stored
exponential histograms. Stored series that change to a representation the
selector does not read are marked stale there.
__debug_stored_as__="true" adds a normal __stored_as__ label with the
representation each series is stored as, splitting stored series whose
representation changes.
Both control labels are removed before selecting from the storage. A
selector still needs a non-empty matcher besides them.
Signed-off-by: Bartek Plotka <bwplotka@google.com>
Where a selector read a histogram both stored and converted, e.g. in a range covering a migration or while always_scrape_classic_histograms is enabled, both were returned. Queries failed with "vector cannot contain metrics with the same labelset", and aggregations counted the histogram twice. Now stored data wins. Converted samples are dropped at the timestamps of the stored samples of the same histogram, which for classic histograms means the same labels except le, and the rest is merged into the stored series with the same labels. A converted series gets a staleness marker where the stored histogram takes over, so that buckets converted from an exponential histogram don't linger until the end of the lookback window. A staleness marker of one side of a merged series only ends that side, so the markers the scrape loop writes when a histogram switches representation don't hide the other one. Selectors with a le matcher do one more select to find the timestamps of the stored histogram. Series are now returned sorted by labels. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…ionFrom EnableHistogramConversion was redundant with HistogramConversionFrom, so it's removed, and an empty HistogramConversionFrom disables query-time histogram conversion. This means --enable-feature=promql-histogram-conversion no longer enables the __convert_stored_as__ and __debug_stored_as__ matchers on its own. It has no effect, and logs a warning, until --query.convert-histograms-from lists at least one representation. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…as__="" A __convert_stored_as__ matcher that matched none of the representations selected nothing, including __convert_stored_as__="". Now matchers that match the empty value, but none of the representations, turn the conversion off for the selector, which returns the stored series as they are. That's a safe way to rule out the conversion, e.g. when debugging a query, and it's also what such a selector returns where the feature is disabled, as no stored series has the label. __debug_stored_as__="true" still adds the __stored_as__ label. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…owners Signed-off-by: Bartek Plotka <bwplotka@google.com>
bwplotka
commented
Sep 27, 2026
The querier selected the series to convert from, and the stored series without the le matchers, on the first Next of the returned series set. The PromQL engine wraps the fanout querier, which merges remote read storage as secondary queriers, and those panic if Select is called after the first Next of any series set they returned. So every converting selector panicked with remote_read configured. Select now selects all series upfront, and Next only reads them. The cost is that a _bucket selector with le matchers that converts from native histograms selects the stored series without the le matchers, even if nothing ends up converted. Also document that only PromQL uses the querier, so the remote read endpoint returns stored data only. Signed-off-by: Bartek Plotka <bwplotka@google.com>
The cache holds full label sets, including e.g. job and instance, but it was only reset when the metric name changed. Reusing it for another series of the same metric emitted the labels of the previous series. Reset it whenever the labels change instead. TestConvertExponentialToClassic now shares one cache across all cases, which fails without the fix. Signed-off-by: Bartek Plotka <bwplotka@google.com>
AllBucketIterator returns the empty buckets within spans, e.g. the ones the TSDB keeps. AppendClassicBoundaries added their boundaries, so the result depended on the span layout. An empty bucket within the zero bucket, whose lower boundary the iterator clamps to the zero threshold, even added a boundary within the zero bucket, which is neither exact nor ascending, so ConvertExponentialToClassic rejected the boundaries. Skip empty buckets, so they count as a gap. Signed-off-by: Bartek Plotka <bwplotka@google.com>
TestConvertNHCBToClassicHistogram_CacheMatchesNoCache claimed to re-run every case of TestConvertNHCBToClassicHistogram through a cache, but only converted one histogram. Run every case of the table for every onlySuffix, with and without a cache shared across all cases, like TestConvertExponentialToClassic does, and drop the separate test. Also update the benchmark comment, which still referred to storage/nhcb_querier.go. Signed-off-by: Bartek Plotka <bwplotka@google.com>
No test case gave the stored series of a classic histogram different timestamps, so the union of their timestamps in addTimestamps was not covered. Signed-off-by: Bartek Plotka <bwplotka@google.com>
… from shadow Where shadow dropped the first converted samples, or replaced a dropped one with a staleness marker, a staleness marker of the converted series right after it was returned as the first sample, or right after another staleness marker. Signed-off-by: Bartek Plotka <bwplotka@google.com>
mergeSamples returned a staleness marker right after another one, e.g. where the stored side went stale and the converted side went stale after it. Drop staleness markers that would be returned first or right after another one. Where the merged samples switch between stored and converted data, the counter reset hint of a native histogram refers to the sample before it on its own side, e.g. the TSDB returns not_reset for all but the first histogram of a chunk. Reset such hints to unknown, unless the histogram is a gauge histogram, like the chain sample iterator of storage.ChainedSeriesMerge does. Signed-off-by: Bartek Plotka <bwplotka@google.com>
toClassic and toNHCB dropped the start timestamps of the samples they converted, unlike the stored samples, so functions like rate() couldn't use them with UseStartTimestamps enabled. Classic histogram samples converted from a native histogram get its start timestamp. An NHCB gets the latest start timestamp of the classic histogram samples it was converted from, which usually all have the same one. Signed-off-by: Bartek Plotka <bwplotka@google.com>
toClassic only validated native histograms when converting them, after it derived the classic bucket boundaries of exponential histograms. So a corrupt stored histogram, e.g. with fewer buckets than its spans need, made CopyToSchema panic. Validate native histograms before anything reads their buckets, and skip invalid ones like the classic histograms that cannot be converted to NHCB: the series converted from them are marked stale, and a new NativeToClassicConversionWarning annotation reports them. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…rkers toNHCB marked the NHCB stale at every timestamp where all of its classic series were stale. So it returned leading and consecutive staleness markers where the classic series had them, or had them around a classic histogram that could not be converted. Signed-off-by: Bartek Plotka <bwplotka@google.com>
seriesSummary only printed sample timestamps, and the stored NHCB of the "stored native histogram wins" case had the same values as the classic histogram at that timestamp. So the test passed even if converted data overwrote stored data. Use samplesSummary, which prints the values, give the stored NHCB other values than the classic histogram, and drop seriesSummary. Signed-off-by: Bartek Plotka <bwplotka@google.com>
Selectors with le matchers now select the stored series without them upfront, even if nothing is converted. Signed-off-by: Bartek Plotka <bwplotka@google.com>
Previously, classic _bucket queries with le matchers issued an extra Select without the le matchers just to collect the timestamps of all stored classic buckets so stored-wins could suppress converted buckets whose le value did not exist in the stored classic histogram. Remove that extra Select and use only the timestamps of the stored series actually selected by the query. If a query asks for a bucket bound that does not exist as a stored classic series, converted buckets for that bound are returned at timestamps where the source native histogram has samples. Signed-off-by: Bartek Plotka <bwplotka@google.com>
…where possible Previously, every converted selector issued two Select calls to the wrapped querier: one for the stored series and one for the series to convert from. When selecting native histograms, or classic histogram series without le matchers, select both the stored series and the series to convert from in a single Select call using a metric name regexp matcher such as __name__=~"foo(|_bucket)" or __name__=~"foo(|_bucket|_count|_sum)". These patterns decompose into exact set matches in FastRegexMatcher, so the TSDB looks up their postings directly. Only selectors for classic histogram series with le matchers still issue a second Select call, so the storage filters the stored series by le. Signed-off-by: Bartek Plotka <bwplotka@google.com>
Signed-off-by: Bartek Plotka <bwplotka@google.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Spec: https://github.com/bwplotka/prometheus/blob/histogram-promql/storage/histogramconv/PROPOSAL.md