Parse created timestamps from OpenMetrics-Text format #13506

ArthurSens · 2024-01-31T23:19:41Z

The already existing feature-flag 'created-timestamp-zero-ingestion' is extended to also cover OpenMetrics-Text format.
Once enabled, _created lines, that before were treated as a whole new timeseries, will be appended as a synthetic zero to the timeseries they are related to.

csmarchbanks · 2024-02-07T15:11:40Z

model/textparse/openmetricsparse.go

+// The difference between an usual metric and the created timestamp is that for
+// the created timestamp, the metric name ends with "_created".
+//
+// Created timestamps are always exposed right after the metric they are


right after

Is that part of the OpenMetrics spec? I think in practice I have only seen _created as the last metric in a MetricFamily, but I haven't read anything in the spec that requires that. It does have to be with the other samples, but e.g. _created before _total might be possible?

Looking at the example in their specification, I see _created lines for each metric point and not MetricFamily.

In the section Overall Structure, we have one created line for acme_http_router_request_seconds_created{path="/api/v1",method="GET"} and another for acme_http_router_request_seconds_created{path="/api/v2",method="POST"} (notice the label difference while using the same name)

but e.g. _created before _total might be possible

Do you mean for the same MetricPoint? All the examples I've seen have the _created line as the last line of a MetricPoint, but I couldn't find anything in the spec saying that this order is a MUST. I'm not sure if we should expect something different 🤔

You're correct, I meant MetricPoint, sorry!

I agree that all of the samples show _created as the last line, but that doesn't seem enforced so I think it is possible and valid OpenMetrics for _created to come first.

😭😭😭😭

That makes things so much more complicated 🥲

Yeah, it might be worth getting an opinion from someone more involved in OpenMetrics than myself as I agree, it would be nice not to have to worry about the order!

I'm not more involved in OpenMetrics, but I think it's totally fine that an experimental feature relies on this accidental property.

Yea, the order of _created is not guaranteed to be the last. But also I don't understand you worries @ArthurSens -- if it's last we already have the problem of having to "buffer" previous samples related to the _created metric which is potentially at the end. So it does not matter if it's end, middle or first. We still have to buffer samples (and corresponding series, which should be the same) no matter what, so we can add potential 0 sample OR in future add it to metadata.

Right now CreatedTimestamp() returns number ONLY when parser hits _created metric, which is clearly not enough for OM as we already written sample for related series no? We have to change bit this logic to e.g. buffer series in (OpenMetrics parsing only).

To me that means e.g. having different append function all together for OM, and that's fine. It should motivate, heavy changes to newer OM protocol to fix this.

I'm not 100% sure yet(doing research at the moment), but I believe all official client libraries that offer OpenMetrics support today expose _created lines after the related metric. If I can confirm this, and seeing that this feature is experimental, could we rely on this order for this PR?

Expecting different order for the _created lines will make the code a lot more complex, we could make reviews easier if we split this up in different work streams :)

We can, but my point stays Arthur, how it makes the code more complex if it's before , in the middle or after the line? The main challenge is that you have to keep buffering samples somewhere BEFORE appending until you know that _created line. This is because we try to be lean and stream parsing line by line.

This is the main complexity to the code, not searching where that line is or something 🤔 I also don't see that buffering in this PR, can you elaborate a planned algorithm for parsing with OM text then you envision (with and without assumption of _created line being last)? Maybe in pseudocode here?

bboreham

I had a few stylistic thoughts.
Note there are some merge clashes , also a CI error.

model/textparse/interface.go

scrape/scrape.go

bboreham · 2024-02-28T17:09:03Z

scrape/scrape.go

-		if et, err = p.Next(); err != nil {
-			if errors.Is(err, io.EOF) {
-				err = nil
+		if !skipCallNextEntry {


Wondering if it would be simpler to re-arrange so Next has always been called at the start of the loop?

I can't see how that could be done... (or maybe I couldn't understand what you mean)

For OM-text, we need to parse 2 lines to identify created timestamps and the parser can't go back once a line is parsed, therefore we need to skip parsing yet another line if we haven't found a _created line in the last call 🤔

ArthurSens · 2024-03-02T18:43:49Z

whooops, a bad push accidentally closed the PR 😅

ArthurSens · 2024-03-02T19:00:40Z

I can see that the changes I've made have broken the fuzz testing, but I do not understand how I'm supposed to fix it 🤔, could someone point me in the right direction?

And the go tests are passing locally... not sure if this one is flaky or not 😬

SuperQ · 2024-03-04T11:10:14Z

I think the Go test is currently flakey, I'm seeing various build failures on other PRs.

SuperQ · 2024-03-07T10:08:54Z

@ArthurSens

# github.com/prometheus/prometheus/promql
./fuzz.go:64:16: assignment mismatch: 2 variables but textparse.New returns 3 values

Looks like you just need to update /promql/fuzz.go.

func fuzzParseMetricWithContentType(in []byte, contentType string) int {
  p, _, warning := textparse.New(in, contentType, false, symbolTable)

ArthurSens · 2024-03-07T14:29:37Z

@ArthurSens

# github.com/prometheus/prometheus/promql
./fuzz.go:64:16: assignment mismatch: 2 variables but textparse.New returns 3 values

Looks like you just need to update /promql/fuzz.go.

func fuzzParseMetricWithContentType(in []byte, contentType string) int {
  p, _, warning := textparse.New(in, contentType, false, symbolTable)

Oh wow, not sure how my IDE didn't catch that 🤔
Thanks for that!

ArthurSens · 2024-03-30T18:12:07Z

Conflicts resolved again, reviews are appreciated 😅

bwplotka

Good start! Thanks! Sorry for delays in reviews.

However, something is off with the current flow for OM text, shouldn't we buffer appends or potential series related to _created metric?

Let's figure this out https://github.com/prometheus/prometheus/pull/13506/files#r1506306215

cmd/prometheus/main.go

model/textparse/interface.go

model/textparse/openmetricsparse.go

bwplotka · 2024-04-05T08:47:45Z

model/textparse/openmetricsparse_test.go

@@ -64,7 +64,10 @@ _metric_starting_with_underscore 1
 testmetric{_label_starting_with_underscore="foo"} 1
 testmetric{label="\"bar\""} 1
 # TYPE foo counter
-foo_total 17.0 1520879607.789 # {id="counter-test"} 5`
+foo_total 17.0 1520879607.789 # {id="counter-test"} 5


Let's change order of _created to explictly show (and test) how it will be handled.

bwplotka · 2024-04-05T08:49:11Z

scrape/scrape.go

-					// CT is an experimental feature. For now, we don't need to fail the
-					// scrape on errors updating the created timestamp, log debug.
-					level.Debug(sl.l).Log("msg", "Error when appending CT in scrape loop", "series", string(met), "ct", *ctMs, "t", t, "err", err)
+			if sl.enableCTZeroIngestion {


I would suggest burning that _created metric in fire in this mode (not ingest it)

Sorry, I didn't get it 😬 what do you mean with "in this mode"?

Maybe you are (trying) to do this already with that skip line? (in this mode == when CT zero ingestion, so CT handling is enabled. if enabled we should probably kill _created metrics too once used).

scrape/scrape.go

bwplotka · 2024-04-05T09:08:50Z

model/textparse/openmetricsparse.go

+// The difference between an usual metric and the created timestamp is that for
+// the created timestamp, the metric name ends with "_created".
+//
+// Created timestamps are always exposed right after the metric they are


Yea, the order of _created is not guaranteed to be the last. But also I don't understand you worries @ArthurSens -- if it's last we already have the problem of having to "buffer" previous samples related to the _created metric which is potentially at the end. So it does not matter if it's end, middle or first. We still have to buffer samples (and corresponding series, which should be the same) no matter what, so we can add potential 0 sample OR in future add it to metadata.

Right now CreatedTimestamp() returns number ONLY when parser hits _created metric, which is clearly not enough for OM as we already written sample for related series no? We have to change bit this logic to e.g. buffer series in (OpenMetrics parsing only).

To me that means e.g. having different append function all together for OM, and that's fine. It should motivate, heavy changes to newer OM protocol to fix this.

Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>

ArthurSens mentioned this pull request Feb 2, 2024

Write created lines when negotiating OpenMetrics prometheus/common#504

Merged

ArthurSens force-pushed the om-text-ctparser branch from a529cad to 78a9972 Compare February 2, 2024 22:09

ArthurSens marked this pull request as draft February 2, 2024 22:11

ArthurSens force-pushed the om-text-ctparser branch from 6743011 to 424b6bf Compare February 4, 2024 15:18

ArthurSens changed the title ~~[WIP] Implement Created Timestamp parsing for OM text parser~~ Parse created timestamps from OpenMetrics-Text format Feb 4, 2024

ArthurSens marked this pull request as ready for review February 4, 2024 15:19

ArthurSens force-pushed the om-text-ctparser branch from 424b6bf to e3ffab3 Compare February 4, 2024 15:21

csmarchbanks reviewed Feb 7, 2024

View reviewed changes

ArthurSens mentioned this pull request Feb 12, 2024

Variant of increase function that assumes uninitialized counters start at zero 0, like VictoriaMetric's increase_pure #13570

Closed

bboreham reviewed Feb 28, 2024

View reviewed changes

ArthurSens closed this Mar 2, 2024

ArthurSens force-pushed the om-text-ctparser branch from e3ffab3 to e79b9ed Compare March 2, 2024 18:41

ArthurSens reopened this Mar 2, 2024

ArthurSens force-pushed the om-text-ctparser branch from 6282cf4 to 419ed90 Compare March 2, 2024 18:55

ArthurSens requested a review from roidelapluie as a code owner March 7, 2024 14:30

SuperQ requested review from bboreham and csmarchbanks March 7, 2024 19:32

ArthurSens force-pushed the om-text-ctparser branch from cb0dc26 to a7226df Compare March 30, 2024 17:56

bwplotka requested changes Apr 5, 2024

View reviewed changes

ArthurSens force-pushed the om-text-ctparser branch 2 times, most recently from 56772d3 to f189fd0 Compare April 5, 2024 15:02

Implement OpenMetrics-text's created timestamp parser

d19875d

Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>

ArthurSens force-pushed the om-text-ctparser branch from f189fd0 to d19875d Compare April 5, 2024 17:19

alshain mentioned this pull request Apr 23, 2024

OpenMetrics _created timestamp micrometer-metrics/micrometer#2625

Open

ArthurSens mentioned this pull request May 21, 2024

tsdb: need histogram support for created-timestamp handling #13384

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse created timestamps from OpenMetrics-Text format #13506

Parse created timestamps from OpenMetrics-Text format #13506

ArthurSens commented Jan 31, 2024 •

edited

csmarchbanks Feb 7, 2024 •

edited

ArthurSens Feb 7, 2024

csmarchbanks Feb 7, 2024

ArthurSens Feb 7, 2024

csmarchbanks Feb 7, 2024

bboreham Feb 28, 2024

bwplotka Apr 5, 2024

ArthurSens Apr 5, 2024

bwplotka Apr 9, 2024

bboreham left a comment

bboreham Feb 28, 2024

ArthurSens Mar 2, 2024

ArthurSens commented Mar 2, 2024

ArthurSens commented Mar 2, 2024 •

edited

SuperQ commented Mar 4, 2024

SuperQ commented Mar 7, 2024 •

edited

ArthurSens commented Mar 7, 2024

ArthurSens commented Mar 30, 2024

bwplotka left a comment

bwplotka Apr 5, 2024

bwplotka Apr 5, 2024

ArthurSens Apr 5, 2024

bwplotka Apr 9, 2024

bwplotka Apr 5, 2024

Parse created timestamps from OpenMetrics-Text format #13506

Are you sure you want to change the base?

Parse created timestamps from OpenMetrics-Text format #13506

Conversation

ArthurSens commented Jan 31, 2024 • edited

csmarchbanks Feb 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens commented Mar 2, 2024

ArthurSens commented Mar 2, 2024 • edited

SuperQ commented Mar 4, 2024

SuperQ commented Mar 7, 2024 • edited

ArthurSens commented Mar 7, 2024

ArthurSens commented Mar 30, 2024

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens commented Jan 31, 2024 •

edited

csmarchbanks Feb 7, 2024 •

edited

ArthurSens commented Mar 2, 2024 •

edited

SuperQ commented Mar 7, 2024 •

edited