[TEST] Scraping: Add microbenchmarks for OM CT parsing by Maniktherana · Pull Request #14933 · prometheus/prometheus

Maniktherana · 2024-09-18T12:39:16Z

partially address #14823

Adds openmetrics with ct parsing enabled to BenchmarkParse

cc: @bwplotka @ArthurSens

Signed-off-by: Manik Rana <manikrana54@gmail.com>

model/textparse/promparse_test.go

Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com> Signed-off-by: Manik Rana <Manikrana54@gmail.com>

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana · 2024-09-25T14:10:53Z

Still unsure about approach here
I believe it makes sense that I keep benchmarking for CreatedTimestamp() function separate from BenchmarkParse since the former only benchmark's a specific portion of the parser

But I can still add the OM parser with skipCT enabled in BenchmarkParse function

ArthurSens · 2024-09-25T18:55:07Z

I've spent some time looking at the benchmarks we have today and it seems like we benchmark both parsers with Prometheus-text exposition format. See how the test data doesn't have _created lines, doesn't have # UNIT lines, etc. It's fine benchmarking both Prometheus and OpenMetrics parsers since the Prometheus text format is a subset of OpenMetrics.

Fundamentally what we want to benchmark with this PR are things that differentiate OM from Prometheus format, so I think it makes sense to write a separate Benchmark function.

What do you think of renaming BenchmarkParse to BenchmarkPrometheusTxtParse and then writing a BenchmarkOpenMetricsTxtParse? The new benchmark will not use the current testdata, and the old benchmarks will not use the new testdata, this way we avoid a lot of conditionals that you had to add at the moment

Maniktherana · 2024-09-25T18:58:47Z

What do you think of renaming BenchmarkParse to BenchmarkPrometheusTxtParse and then writing a BenchmarkOpenMetricsTxtParse? The new benchmark will not use the current testdata, and the old benchmarks will not use the new testdata, this way we avoid a lot of conditionals that you had to add at the moment

Sounds good to me

will BenchmarkPrometheusTxtParse have both OM parsers (CT enabled and disabled)? or is that just for BenchmarkOpenMetricsTxtParse

ArthurSens · 2024-09-25T19:22:00Z

Sounds good to me

will BenchmarkPrometheusTxtParse have both OM parsers (CT enabled and disabled)? or is that just for BenchmarkOpenMetricsTxtParse

I would keep only the default behavior (not skipping CT) for PrometheusTxtParse.

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana · 2024-09-26T07:19:45Z

Sounds good to me
will BenchmarkPrometheusTxtParse have both OM parsers (CT enabled and disabled)? or is that just for BenchmarkOpenMetricsTxtParse

I would keep only the default behavior (not skipping CT) for PrometheusTxtParse.

pushed the changes

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ArthurSens

Small nit, but LGTM!

@bwplotka could you take a look as well?

model/textparse/openmetricsparse_test.go

Signed-off-by: Manik Rana <manikrana54@gmail.com>

model/textparse/promparse_test.go

model/textparse/openmetricsparse_test.go

bwplotka

Thanks, nice!

I wonder if it would make sense to actually clean BenchmarkParse. Perhaps put it in separate benchmark_test.go file, and have a clear 3 cases that always call Created Timestamp (with https://www.bwplotka.dev/2024/go-microbenchmarks-benchstat/ case naming):

for _, in := range []string{"omtestdata.txt", "promtestdata.txt", "promtestdata.nometa.txt"} {
b.Run(fmt.Sprintf("input=%v"), in, func....)
}

Then chose the parser based on input format.

Then you can have cases like case=Series, case=CreatedTimestamp and case=Series+Metric based on methods you are calling.

Does it make sense to have benchmark for skip true and false? Is this really that different between those? I think the main problem is CreatedTimestamp implementation right now, no matter if we skip or not, and I would capture the benchmark for default path (skip = true). We can do adhoc benchmark run with skip = false for testing, but not need to capture this perhaps?

Maniktherana · 2024-10-01T17:57:01Z

Does it make sense to have benchmark for skip true and false? Is this really that different between those? I think the main problem is CreatedTimestamp implementation right now, no matter if we skip or not, and I would capture the benchmark for default path (skip = true). We can do adhoc benchmark run with skip = false for testing, but not need to capture this perhaps?

Yes this makes sense. I'll make some changes

Although I still feel we can keep a benchmarkParse in omparse and promparse based off which parser we use
(But that's just me and I could be wrong)

bwplotka · 2024-10-01T17:58:01Z

Although I still feel we can keep a benchmarkParse in omparse and promparse based off which parser we use
(But that's just me and I could be wrong)

Is this duplication of benchmark code (that ages quickly) worth it? Why?

bwplotka

Actually, let's do it like that, just name the omtext benchmark a bit more explicitly (: We can improve this one later.

Thanks! LGTM

model/textparse/openmetricsparse_test.go

bwplotka · 2024-10-01T18:21:58Z

model/textparse/openmetricsparse_test.go

+		"openmetrics": func(b []byte, st *labels.SymbolTable) Parser {
+			return NewOpenMetricsParser(b, st)
+		},
+		"openmetrics-skip-ct": func(b []byte, st *labels.SymbolTable) Parser {


Perhaps it would make sense here to always do openmetrics (no case like that) but have b.Run(fmt.Sprintf("skip-ct=%v", skipCT), ....) case, WDYT?

yes I agree with that bit
virtually no difference here for both parsers

Happy to do in separare PR

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manik Rana <Manikrana54@gmail.com>

bwplotka · 2024-10-02T06:48:10Z

model/textparse/openmetricsparse_test.go

+		"openmetrics": func(b []byte, st *labels.SymbolTable) Parser {
+			return NewOpenMetricsParser(b, st)
+		},
+		"openmetrics-skip-ct": func(b []byte, st *labels.SymbolTable) Parser {


Happy to do in separare PR

test: benchmark OM CT parsing

3a2d8d5

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana changed the title ~~Microbenchmark OM CT parsing~~ feat: add microbenchmarks for OM CT parsing Sep 18, 2024

refac: move OM ct benchmark to promparse_test

4f0e6b9

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana marked this pull request as ready for review September 20, 2024 06:26

ArthurSens reviewed Sep 20, 2024

View reviewed changes

model/textparse/promparse_test.go Outdated Show resolved Hide resolved

model/textparse/promparse_test.go Outdated Show resolved Hide resolved

Maniktherana and others added 3 commits September 20, 2024 19:18

chore: stricter comparison

b0f5711

Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com> Signed-off-by: Manik Rana <Manikrana54@gmail.com>

feat: use richer OM test data

5172472

Signed-off-by: Manik Rana <manikrana54@gmail.com>

refac: move parse-ct test outside of inner loop

f64b276

Signed-off-by: Manik Rana <manikrana54@gmail.com>

Maniktherana mentioned this pull request Sep 25, 2024

[BUGFIX] Scraping: Naive fixes and optimzations for CreatedTimestamp function #14965

Merged

refac: separate benchmarks for om and prom parsers

0c86e0e

Signed-off-by: Manik Rana <manikrana54@gmail.com>

chore: remove unused code

ac81ab5

Signed-off-by: Manik Rana <manikrana54@gmail.com>

ArthurSens previously approved these changes Sep 26, 2024

View reviewed changes

model/textparse/openmetricsparse_test.go Outdated Show resolved Hide resolved

Maniktherana dismissed ArthurSens’s stale review via ac81ab5 September 26, 2024 17:37

ArthurSens reviewed Sep 26, 2024

View reviewed changes

model/textparse/openmetricsparse_test.go Outdated Show resolved Hide resolved

chore: remove more unused code

c6ccf00

Signed-off-by: Manik Rana <manikrana54@gmail.com>

bwplotka reviewed Oct 1, 2024

View reviewed changes

model/textparse/promparse_test.go Show resolved Hide resolved

bwplotka reviewed Oct 1, 2024

View reviewed changes

model/textparse/promparse_test.go Show resolved Hide resolved

bwplotka reviewed Oct 1, 2024

View reviewed changes

model/textparse/openmetricsparse_test.go Show resolved Hide resolved

bwplotka requested changes Oct 1, 2024

View reviewed changes

bwplotka previously approved these changes Oct 1, 2024

View reviewed changes

model/textparse/openmetricsparse_test.go Outdated Show resolved Hide resolved

bwplotka reviewed Oct 1, 2024

View reviewed changes

Maniktherana dismissed bwplotka’s stale review via c976300 October 1, 2024 18:34

refac: rename to BenchmarkOMParseCreatedTimestamp

c976300

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Manik Rana <Manikrana54@gmail.com>

bwplotka approved these changes Oct 2, 2024

View reviewed changes

bwplotka merged commit 98cd80b into prometheus:main Oct 2, 2024

Maniktherana deleted the benchmark-ct-om branch October 2, 2024 11:17

bboreham changed the title ~~feat: add microbenchmarks for OM CT parsing~~ [TEST] Scraping: Add microbenchmarks for OM CT parsing Oct 8, 2024

Maniktherana mentioned this pull request Oct 12, 2024

Talk proposal: Breaking and optimizing the Prometheus parser pydelhi/talks#300

Closed

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

feat: add microbenchmarks for OM CT parsing (prometheus#14933)

d5c444f

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Maniktherana commented Sep 25, 2024

Uh oh!

ArthurSens commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maniktherana commented Sep 25, 2024

Uh oh!

ArthurSens commented Sep 25, 2024

Uh oh!

Maniktherana commented Sep 26, 2024

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwplotka left a comment • 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!

Maniktherana commented Oct 1, 2024

Uh oh!

bwplotka commented Oct 1, 2024

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka Oct 1, 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!

Maniktherana Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

bwplotka Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

bwplotka Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Maniktherana commented Sep 18, 2024 •

edited

Loading

ArthurSens commented Sep 25, 2024 •

edited

Loading

bwplotka left a comment •

edited

Loading

bwplotka Oct 1, 2024 •

edited

Loading