feat(nhcb): implement created timestamp handling by krajorama · Pull Request #15198 · prometheus/prometheus

krajorama · 2024-10-23T06:54:58Z

Follows #14978
Related to #13529
Fixes: #15137

Implement missing created timestamp handling

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Call through to the underlaying parser if we are not in a histogram and the entry is a series or exponential native histogram. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Fixes: #15137 Ignore exemplars while peeking ahead during CT parsing. Simplify state reset with defer(). Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama · 2024-10-23T08:38:40Z

model/textparse/interface_test.go

 			if ct := p.CreatedTimestamp(); ct != nil {
 				got.ct = int64p(*ct)
 			}
+			for e := (exemplar.Exemplar{}); p.Exemplar(&e); {


Note to reviewers: this reordering is done to trigger #15137. And also means the tests use the same order that scrape.go actually does things.

krajorama · 2024-10-23T12:39:34Z

Improved OM parser performance due to not fully parsing exemplars twice.
Overhead of CT on NHCB parser is +15%.

goos: linux
goarch: amd64
pkg: github.com/prometheus/prometheus/model/textparse
cpu: AMD Ryzen 7 4700U with Radeon Graphics         
                                                         │  main.txt   │               pr.txt               │
                                                         │   sec/op    │   sec/op     vs base               │
Parse/data=promtestdata.txt/parser=promtext-2              312.3µ ± 2%   315.2µ ± 3%        ~ (p=0.485 n=6)
Parse/data=promtestdata.txt/parser=xpfmt-2                 1.642m ± 5%   1.693m ± 4%        ~ (p=0.180 n=6)
Parse/data=promtestdata.nometa.txt/parser=promtext-2       238.7µ ± 2%   242.3µ ± 5%        ~ (p=0.589 n=6)
Parse/data=promtestdata.nometa.txt/parser=xpfmt-2          1.119m ± 6%   1.143m ± 3%        ~ (p=0.818 n=6)
Parse/data=createTestProtoBuf()/parser=promproto-2         103.6µ ± 7%   104.9µ ± 5%        ~ (p=0.699 n=6)
Parse/data=omtestdata.txt/parser=omtext-2                  66.92µ ± 3%   64.53µ ± 4%   -3.57% (p=0.026 n=6)
Parse/data=promtestdata.txt/parser=omtext-2                4.053m ± 1%   3.619m ± 2%  -10.72% (p=0.002 n=6)
Parse/data=omhistogramdata.txt/parser=omtext-2             358.8µ ± 2%   340.9µ ± 1%   -5.01% (p=0.002 n=6)
Parse/data=omhistogramdata.txt/parser=omtext_with_nhcb-2   127.0µ ± 6%   147.0µ ± 4%  +15.72% (p=0.002 n=6)
geomean                                                    382.5µ        384.0µ        +0.37%

                                                         │   main.txt    │               pr.txt                │
                                                         │      B/s      │     B/s       vs base               │
Parse/data=promtestdata.txt/parser=promtext-2               101.9Mi ± 2%   100.9Mi ± 3%        ~ (p=0.485 n=6)
Parse/data=promtestdata.txt/parser=xpfmt-2                  19.39Mi ± 5%   18.79Mi ± 4%        ~ (p=0.167 n=6)
Parse/data=promtestdata.nometa.txt/parser=promtext-2       100.93Mi ± 2%   99.43Mi ± 5%        ~ (p=0.589 n=6)
Parse/data=promtestdata.nometa.txt/parser=xpfmt-2           21.55Mi ± 6%   21.09Mi ± 3%        ~ (p=0.818 n=6)
Parse/data=createTestProtoBuf()/parser=promproto-2          35.39Mi ± 7%   34.96Mi ± 5%        ~ (p=0.623 n=6)
Parse/data=omtestdata.txt/parser=omtext-2                   63.81Mi ± 3%   66.17Mi ± 4%   +3.70% (p=0.026 n=6)
Parse/data=promtestdata.txt/parser=omtext-2                 7.849Mi ± 1%   8.793Mi ± 2%  +12.03% (p=0.002 n=6)
Parse/data=omhistogramdata.txt/parser=omtext-2              11.14Mi ± 2%   11.73Mi ± 1%   +5.27% (p=0.002 n=6)
Parse/data=omhistogramdata.txt/parser=omtext_with_nhcb-2    31.47Mi ± 7%   27.19Mi ± 4%  -13.61% (p=0.002 n=6)
geomean                                                     31.03Mi        30.91Mi        -0.39%

                                                         │   main.txt   │                pr.txt                │
                                                         │     B/op     │     B/op      vs base                │
Parse/data=promtestdata.txt/parser=promtext-2              57.90Ki ± 0%   57.90Ki ± 0%       ~ (p=1.000 n=6)
Parse/data=promtestdata.txt/parser=xpfmt-2                 536.8Ki ± 0%   536.8Ki ± 0%       ~ (p=0.623 n=6)
Parse/data=promtestdata.nometa.txt/parser=promtext-2       57.07Ki ± 0%   57.07Ki ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=promtestdata.nometa.txt/parser=xpfmt-2          381.1Ki ± 0%   381.1Ki ± 0%       ~ (p=0.784 n=6)
Parse/data=createTestProtoBuf()/parser=promproto-2         40.14Ki ± 0%   40.14Ki ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=omtestdata.txt/parser=omtext-2                  11.06Ki ± 0%   11.06Ki ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=promtestdata.txt/parser=omtext-2                109.9Ki ± 0%   109.9Ki ± 0%       ~ (p=0.545 n=6)
Parse/data=omhistogramdata.txt/parser=omtext-2             24.67Ki ± 0%   24.67Ki ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=omhistogramdata.txt/parser=omtext_with_nhcb-2   41.66Ki ± 0%   42.58Ki ± 0%  +2.20% (p=0.002 n=6)
geomean                                                    68.65Ki        68.82Ki       +0.24%
¹ all samples are equal

                                                         │  main.txt   │               pr.txt                │
                                                         │  allocs/op  │  allocs/op   vs base                │
Parse/data=promtestdata.txt/parser=promtext-2              1.038k ± 0%   1.038k ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=promtestdata.txt/parser=xpfmt-2                 12.35k ± 0%   12.35k ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=promtestdata.nometa.txt/parser=promtext-2        828.0 ± 0%    828.0 ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=promtestdata.nometa.txt/parser=xpfmt-2          10.01k ± 0%   10.01k ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=createTestProtoBuf()/parser=promproto-2          870.0 ± 0%    870.0 ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=omtestdata.txt/parser=omtext-2                   240.0 ± 0%    240.0 ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=promtestdata.txt/parser=omtext-2                2.492k ± 0%   2.492k ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=omhistogramdata.txt/parser=omtext-2              375.0 ± 0%    375.0 ± 0%       ~ (p=1.000 n=6) ¹
Parse/data=omhistogramdata.txt/parser=omtext_with_nhcb-2    506.0 ± 0%    524.0 ± 0%  +3.56% (p=0.002 n=6)
geomean                                                    1.298k        1.303k       +0.39%
¹ all samples are equal

bwplotka

Beautiful, thanks!

Maybe one nit is to include a quick test case for your change in OMParser that would surface the previous regression if it would come back. Do you think this would be useful?

cc @Maniktherana would you like to review too and give your LGTM/comments? 🤗

krajorama · 2024-10-23T15:21:10Z

Maybe one nit is to include a quick test case for your change in OMParser that would surface the previous regression if it would come back. Do you think this would be useful?

The tests already fail on main if you change the order, no new test needed:

--- FAIL: TestOpenMetricsParse (0.01s)
...
        	            	  	... // 18 identical elements
        	            	  	{m: `gh_bucket{le="+Inf"}`, v: 1, lset: s`{__name__="gh_bucket", le="+Inf"}`},
        	            	  	{m: "hhh", typ: "histogram"},
        	            	  	{
        	            	  		... // 4 identical fields
        	            	  		lset: s`{__name__="hhh_bucket", le="+Inf"}`,
        	            	  		t:    nil,
        	            	- 		es:   []exemplar.Exemplar{{Labels: s`{id="histogram-bucket-test"}`, Value: 4}},
        	            	+ 		es:   nil,
        	            	  		ct:   nil,
        	            	  		typ:  "",
        	            	  		... // 3 identical fields
        	            	  	},
        	            	  	{m: "hhh_count", v: 1, lset: s`{__name__="hhh_count"}`, es: {{Labels: s`{id="histogram-count-test"}`, Value: 4}}, ...},
        	            	  	{m: "ggh", typ: "gaugehistogram"},
        	            	  	{m: `ggh_bucket{le="+Inf"}`, v: 1, lset: s`{__name__="ggh_bucket", le="+Inf"}`, es: {{Labels: s`{id="gaugehistogram-bucket-test", xx="yy"}`, Value: 4, Ts: 123123, HasTs: true}}, ...},
        	            	  	{m: "ggh_count", v: 1, lset: s`{__name__="ggh_count"}`, es: {{Labels: s`{id="gaugehistogram-count-test", xx="yy"}`, Value: 4, Ts: 123123, HasTs: true}}, ...},
        	            	  	{m: "smr_seconds", typ: "summary"},
        	            	  	{
        	            	  		... // 4 identical fields
        	            	  		lset: s`{__name__="smr_seconds_count"}`,
        	            	  		t:    nil,
        	            	- 		es:   []exemplar.Exemplar{{Labels: s`{id="summary-count-test"}`, Value: 1, Ts: 123321, HasTs: true}},
        	            	+ 		es:   nil,
        	            	  		ct:   nil,
        	            	  		typ:  "",
        	            	  		... // 3 identical fields
        	            	  	},
        	            	  	{m: "smr_seconds_sum", v: 42, lset: s`{__name__="smr_seconds_sum"}`, es: {{Labels: s`{id="summary-sum-test"}`, Value: 1, Ts: 123321, HasTs: true}}, ...},
        	            	  	{m: "ii", typ: "info"},
        	            	  	... // 9 identical elements
        	            	  	{m: "foo", help: "Counter with and without labels to certify CT is parsed for both"...},
        	            	  	{m: "foo", typ: "counter"},
        	            	  	{
        	            	  		... // 4 identical fields
        	            	  		lset: s`{__name__="foo_total"}`,
        	            	  		t:    &1520879607789,
        	            	- 		es:   []exemplar.Exemplar{{Labels: s`{id="counter-test"}`, Value: 5}},
        	            	+ 		es:   nil,
        	            	  		ct:   &1520872607123,
        	            	  		typ:  "",
        	            	  		... // 3 identical fields
        	            	  	},
        	            	  	{
        	            	  		... // 4 identical fields
        	            	  		lset: s`{__name__="foo_total", a="b"}`,
        	            	  		t:    &1520879607789,
        	            	- 		es:   []exemplar.Exemplar{{Labels: s`{id="counter-test"}`, Value: 5}},
        	            	+ 		es:   nil,
        	            	  		ct:   &1520872607123,
        	            	  		typ:  "",
        	            	  		... // 3 identical fields
        	            	  	},
        	            	  	{m: `foo_total{le="c"}`, v: 21, lset: s`{__name__="foo_total", le="c"}`, ct: &1520872621123, ...},
        	            	  	{m: `foo_total{le="1"}`, v: 10, lset: s`{__name__="foo_total", le="1"}`},
        	            	  	... // 36 identical elements
        	            	  }
        	Test:       	TestOpenMetricsParse

Maniktherana · 2024-10-23T15:49:42Z

model/textparse/openmetricsparse.go

+	defer func() {
+		p.ignoreExemplar = false
+		p.start = savedStart
+		p.l = resetLexer


why didn't I think of that

LGTM!

krajorama added 4 commits October 23, 2024 08:52

Refactor nhcbparse Next to consistently handle p.entry and p.err

2409ef3

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

NHCB scrape: make created timestamp work with non classic histograms

15cd69c

Call through to the underlaying parser if we are not in a histogram and the entry is a series or exponential native histogram. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

fix(omparser): losing exemplars when CT is parsed

4fb49e5

Fixes: #15137 Ignore exemplars while peeking ahead during CT parsing. Simplify state reset with defer(). Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

feat(nhcb): implement CT for all types in parser

a67a243

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama commented Oct 23, 2024

View reviewed changes

krajorama marked this pull request as ready for review October 23, 2024 08:39

krajorama requested review from beorn7, bwplotka, carrieedwards and fionaliao October 23, 2024 08:39

bwplotka approved these changes Oct 23, 2024

View reviewed changes

Maniktherana approved these changes Oct 23, 2024

View reviewed changes

krajorama merged commit 2182b83 into main Oct 24, 2024

krajorama deleted the nhcb-scrape-ct branch October 24, 2024 05:39

julienduchesne pushed a commit to julienduchesne/prometheus that referenced this pull request Dec 13, 2024

feat(nhcb): implement created timestamp handling (prometheus#15198)

b11a5ce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nhcb): implement created timestamp handling#15198

feat(nhcb): implement created timestamp handling#15198
krajorama merged 4 commits intomainfrom
nhcb-scrape-ct

krajorama commented Oct 23, 2024 •

edited

Loading

Uh oh!

krajorama Oct 23, 2024 •

edited

Loading

Uh oh!

krajorama commented Oct 23, 2024 •

edited

Loading

Uh oh!

bwplotka left a comment

Uh oh!

krajorama commented Oct 23, 2024 •

edited

Loading

Uh oh!

Maniktherana Oct 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

krajorama commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krajorama Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krajorama commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

krajorama commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maniktherana Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krajorama commented Oct 23, 2024 •

edited

Loading

krajorama Oct 23, 2024 •

edited

Loading

krajorama commented Oct 23, 2024 •

edited

Loading

krajorama commented Oct 23, 2024 •

edited

Loading