textparse: Implement `CreatedTimestamp()` in `openmetricsparse.go` by Maniktherana · Pull Request #14356 · prometheus/prometheus

Maniktherana · 2024-06-27T13:56:05Z

Maniktherana · 2024-06-27T14:03:43Z

Getting a blocker at: https://github.com/prometheus/prometheus/pull/14356/files#diff-0ce7ee179a5b61f32f6202777dc48dd5fdc71bf2b3c4779f78c5e7699384c09eR241

At this line here, even if metric names are theoretically the same, it will still result in an error for test cases like:

# TYPE foo counter
foo_total 123
foo_created 1000

While the metric is foo in both cases how do we confidently get the metric name without the suffix?
it can get even more weird for cases like foo_created{some_label="some_label_value"}

Maniktherana · 2024-07-04T15:42:30Z

I know we covered some options of advancing the parser but is this correct?
I'm able to pass all tests currently (going to add some more soon)

ct := int64(newParser.val)
+_, err := p.Next()
+if err != nil {
+    return nil
+}
return &ct

ArthurSens

I feel like we're pretty close :)

Some test cases for histograms/summaries would be nice as well!

Maniktherana · 2024-07-09T15:00:24Z

@bwplotka @TheSpiritXIII made some changes, Added some tests and they all pass 😳

Maniktherana · 2024-07-09T15:05:42Z

Also, I feel I need some more test cases and I need to confirm some stuff.

Eg:

# TYPE bar summary
bar_count 17.0
bar_sum 324789.3
bar{quantile="0.95"} 123.7
bar{quantile="0.99"} 150.0
bar_created 1520430000

The CreatedTimestamp function only works if the _created line is at the very end of a summary and histogram. Is this how it works or can _created be anywhere within a labelset?

ArthurSens

Good progress! My comments are mostly about readability/simplicity, we're almost there :)

ArthurSens · 2024-07-09T17:07:20Z

+	for {
+		switch t, _ := newParser.Next(); t {
+		case EntrySeries:
+			// TODO: potentially broken? Missing type?


Is this comment still relevant?

Hmm, might still be relevant
We're not sure if TYPE text is optional yet
if it is how do we handle it

bwplotka

Thanks! Great work. Tried to help and proposed improvements, feel free to challenge!

bwplotka · 2024-07-10T09:27:50Z

+	// lines ending with _created are skipped by default as they are
+	// parsed by the CreatedTimestamp method. The skipCT flag is set to
+	// false when the CreatedTimestamp method is called.
+	skipCT bool


This is nice, but it might be too hidden logic from the parser caller perspective. Perhaps less surprising option is to offer a choice on parser constructor for this? Another reason is flexibility, perhaps some parser users would like to ALWAYS skip CT series even without calling CreatedTimestamp? Or opposite, they want to preserve those even when calling CreatedTimestamp.

If it's simple to implement for us, perhaps it makes sense more to use constructor for that decision and make it orthogonal to CreatedTimestamp method use.

NOTE: Likely in Prometheus we will set this to True when CT is enabled, but in future there might option to always set it to true (although one could use relabelling for this)

To be addressed?

About this, essentially whenever we call an OM parser we want skipCT to always be true. The only case the parser will not skip CT is when it is a deepCopy

In that sense I'm not too sure if it makes sense (I could be wrong) to give to option to skipCT via a constructor. This is moreso just a way to track which parser is a deepCopy and which ins't especially when we're calling parser.Next()

I'm failing to understand why this isn't needed as well 😬. From our discussions, I understood that we don't want to add _created lines as a separate new metric. This means that the original parser needs to skip _created lines when calling Next(), but the ephemeral parser in CreatedTimestamp() needs to return it

update to this can be found in this comment: #14356 (comment)

I'm failing to understand why this isn't needed as well 😬 From our discussions, I understood that we don't want to add _created lines as a separate new metric.

Not sure what @ArthurSens mean by "this" now. I only refer to the logic how this field is set. There is quite large and complex explanation in commentry how this is set. I feel it would be easier if we kill this and simply allow constructor to set it. In fact textparse.WithSkipCT was already implemented for other reasons, so we have it. My proposal is to:

keep some default (skip = false?)

ensure that option as we have now e.g textparse.WithOMParserCTSeriesSkipped()

don't change this variable anywhere else (ofc pass true to deep copied for CT logic)

I think this is already implemented by @Maniktherana by a quick look, just I added suggestions below for default and option name 🤗 Thanks!

bwplotka

Epic thanks!

Some comments are still not addressed, checked all of them seems those are still relevant. Otherwise LGTM!

bwplotka · 2024-07-10T20:07:37Z

+	// lines ending with _created are skipped by default as they are
+	// parsed by the CreatedTimestamp method. The skipCT flag is set to
+	// false when the CreatedTimestamp method is called.
+	skipCT bool


To be addressed?

Signed-off-by: Manik Rana <[email protected]>

- implement changes from pair programming session - use newParse.val() - advance parser p if ct is found Signed-off-by: Manik Rana <[email protected]>

Signed-off-by: Manik Rana <[email protected]>

Co-authored-by: Arthur Silva Sens <[email protected]> Signed-off-by: Manik Rana <[email protected]> Signed-off-by: Manik Rana <[email protected]>

Maniktherana · 2024-08-06T05:34:43Z

@bwplotka applied all the suggestions

Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: Manik Rana <[email protected]>

bwplotka

Small nit to go, I feel bad for slow review cycles, so happy to help by proposing PR to this PR or something to unblock you. Added suggestion I think we should make, but, as always, feel free to reject (:

Signed-off-by: Manik Rana <[email protected]>

bwplotka

Nice, thanks! Some comments, but good direction!

Signed-off-by: bwplotka <[email protected]>

bwplotka · 2024-08-07T14:06:32Z

Maniktherana#2

Pair programming with Manik, Arthur and Daniel.

ArthurSens

Looking good, I'm fine with merging as it is but let's keep the momentum and continue the work on small issues in the following weeks :)

Signed-off-by: Manik Rana <[email protected]>

bwplotka · 2024-08-08T11:34:39Z

+}
+
+// TypeRequiresCT returns true if the metric type requires a _created timestamp.
+func TypeRequiresCT(t model.MetricType) bool {


Why public?

its being used in promparse_test so I thought it would be public in this case

bwplotka

Let's merge. One nit, but can be fixed later. Thanks!

bboreham · 2024-09-18T14:10:54Z

+	if contentType == "application/openmetrics-text" {
+		p = textparse.NewOpenMetricsParser(in, symbolTable)
+	}


Why? This appears to be what the code above already does.

this will be removed in my PR here

But this looks like its a relic from an earlier implementation in this PR that I forgot to remove: #14356 (comment)

Maniktherana changed the title ~~feat: initial implement of createedTimestamp() with tests~~ Implement CreatedTimestamp() in openmetricsparse.go Jun 27, 2024

TheSpiritXIII reviewed Jun 27, 2024

View reviewed changes

Comment thread model/textparse/openmetricsparse.go Outdated

TheSpiritXIII reviewed Jun 27, 2024

View reviewed changes

Comment thread model/textparse/openmetricsparse.go

ArthurSens reviewed Jul 5, 2024

View reviewed changes

Comment thread model/textparse/openmetricsparse.go Outdated

ArthurSens reviewed Jul 9, 2024

View reviewed changes

bwplotka requested changes Jul 10, 2024

View reviewed changes

Maniktherana marked this pull request as ready for review July 10, 2024 14:02

Maniktherana force-pushed the createdTimestamp-parser branch from cdd49c9 to dae377b Compare July 10, 2024 16:10

bwplotka previously approved these changes Jul 10, 2024

View reviewed changes

Maniktherana dismissed bwplotka’s stale review via 86404b9 July 11, 2024 05:13

ArthurSens reviewed Jul 11, 2024

View reviewed changes

Comment thread model/textparse/openmetricsparse.go Outdated

Comment thread model/textparse/openmetricsparse_test.go

Comment thread model/textparse/openmetricsparse_test.go Outdated

Maniktherana and others added 15 commits July 12, 2024 00:48

feat: initial implement of createedTimestamp() with tests

7e88992

Signed-off-by: Manik Rana <[email protected]>

feat: return ct after finding it

03f2a90

Signed-off-by: Manik Rana <[email protected]>

chore: remove unneeded test

cbb4e07

Signed-off-by: Manik Rana <[email protected]>

chore: add comments

188d4cc

Signed-off-by: Manik Rana <[email protected]>

feat: multiple changes

e841e00

- implement changes from pair programming session - use newParse.val() - advance parser p if ct is found Signed-off-by: Manik Rana <[email protected]>

fix: check if err from p.Next()

ea9c8cb

Signed-off-by: Manik Rana <[email protected]>

feat: advance parser and parse histograms + summary

96e850d

Signed-off-by: Manik Rana <[email protected]>

fix: restore previous tests

7708b78

Signed-off-by: Manik Rana <[email protected]>

fix: retore failing tests

eba9877

Signed-off-by: Manik Rana <[email protected]>

chore: remove unneeded comments

50615b4

Signed-off-by: Manik Rana <[email protected]>

fix: return nil when mtype doesn't match

4102f41

Signed-off-by: Manik Rana <[email protected]>

chore: update go fmt version

e37930d

Signed-off-by: Manik Rana <[email protected]>

chore: cleanup

8ea8352

Signed-off-by: Manik Rana <[email protected]>

fix: comments

9c4bc80

Signed-off-by: Manik Rana <[email protected]>

feat: document deepcopyparser

e9391dd

Co-authored-by: Arthur Silva Sens <[email protected]> Signed-off-by: Manik Rana <[email protected]> Signed-off-by: Manik Rana <[email protected]>

chore: comments

d4f13f6

Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: Manik Rana <[email protected]>

bwplotka reviewed Aug 7, 2024

View reviewed changes

Comment thread model/textparse/openmetricsparse.go Outdated

Comment thread model/textparse/openmetricsparse.go Outdated

Maniktherana added 3 commits August 7, 2024 16:07

refac: improve error handling

8ccc9af

Signed-off-by: Manik Rana <[email protected]>

fix: return error instead of nil

900c127

Signed-off-by: Manik Rana <[email protected]>

fix: remove skipCT check from tBraceOpen

65e2afd

Signed-off-by: Manik Rana <[email protected]>

bwplotka reviewed Aug 7, 2024

View reviewed changes

Comment thread model/textparse/openmetricsparse.go Outdated

Comment thread model/textparse/openmetricsparse.go Outdated

Comment thread model/textparse/openmetricsparse.go Outdated

Pair programming with Manik, Arthur and Daniel.

0479c46

Signed-off-by: bwplotka <[email protected]>

Merge pull request #2 from prometheus/manik1

acbeb52

Pair programming with Manik, Arthur and Daniel.

ArthurSens previously approved these changes Aug 7, 2024

View reviewed changes

Comment thread model/textparse/openmetricsparse.go

Comment thread model/textparse/openmetricsparse_test.go

Comment thread model/textparse/promparse_test.go Outdated

Comment thread model/textparse/promparse_test.go

chore: comments and use helper funcs

e725fcd

Signed-off-by: Manik Rana <[email protected]>

Maniktherana dismissed ArthurSens’s stale review via e725fcd August 8, 2024 04:49

chore: lint

d0ae7b3

Signed-off-by: Manik Rana <[email protected]>

bwplotka reviewed Aug 8, 2024

View reviewed changes

bwplotka approved these changes Aug 8, 2024

View reviewed changes

bwplotka merged commit 02c465b into prometheus:main Aug 8, 2024

charleskorn mentioned this pull request Aug 12, 2024

Merge upstream at 5fd66ba grafana/mimir-prometheus#678

Merged

ArthurSens mentioned this pull request Aug 12, 2024

Optionally print OM created lines prometheus/client_golang#1408

Merged

bwplotka mentioned this pull request Aug 14, 2024

Parse Created timestamps from OpenMetrics text format #12980

Closed

SuperQ mentioned this pull request Aug 23, 2024

Unwanted _created variables prometheus/client_python#438

Closed

Maniktherana mentioned this pull request Aug 31, 2024

[CHANGE] No longer ingest OM _created as timeseries if feature-flag 'created-timestamp-zero-ingestion' is enabled; fixed OM text CT conversion bug #14738

Merged

This was referenced Sep 2, 2024

OpenMetrics using gigabytes more memory #14808

Closed

fix: check ct ingestion before calling p.CreatedTimestamp #14809

Merged

bwplotka mentioned this pull request Sep 4, 2024

ST: Optimize CPU/memory use for OM1.0 Text CT/ST parsing (st-storage and created-timestamp-zero-ingestion features) #14823

Open

bboreham reviewed Sep 18, 2024

View reviewed changes

Maniktherana mentioned this pull request Sep 20, 2024

chore: remove unused code #14950

Merged

bboreham mentioned this pull request Oct 8, 2024

Prepare 3.0.0-beta.1 #14976

Merged

dashpole mentioned this pull request Nov 20, 2024

prometheusreceiver: Make use of created timestamp from prometheus open-telemetry/opentelemetry-collector-contrib#36473

Closed

tahaderouiche mentioned this pull request Jul 23, 2025

Support created-timestamp-zero-ingestion feature flag for prometheus.scrape component grafana/alloy#4045

Closed

Conversation

Maniktherana commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maniktherana commented Jun 27, 2024

Uh oh!

Uh oh!

Uh oh!

Maniktherana commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Maniktherana commented Jul 9, 2024

Uh oh!

Maniktherana commented Jul 9, 2024

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Maniktherana commented Aug 6, 2024

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Maniktherana commented Jun 27, 2024 •

edited

Loading

Maniktherana commented Jul 4, 2024 •

edited

Loading

ArthurSens left a comment •

edited

Loading