Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[exporter/prometheusremotewrite] Fix data race for batch state by removing the state altogether #36600

Closed

Conversation

ArthurSens
Copy link
Member

@ArthurSens ArthurSens commented Nov 29, 2024

Description

This is an alternative to #36524 and #36601

This PR does a couple of things:

  • Add a test written by @edma2 that shows a data race to the batch state when running multiple consumers.
  • Add a benchmark for PushMetrics, with options to run with a stable number of metrics or varying metrics.
  • Fix the data race by removing the state altogether.

Benchmark results

This is strange to me. I don't see any degradations in memory allocations by removing the state, so I'm confused if the optimization ever made any difference or if I'm doing something wrong in the benchmark.

arthursens$ benchstat main.txt withoutState.txt syncpool.txt 
goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter
cpu: Apple M2 Pro
                            │  main.txt   │       withoutState.txt       │         syncpool.txt         │
                            │   sec/op    │   sec/op     vs base         │   sec/op     vs base         │
PushMetricsVaryingMetrics-2   3.654m ± 6%   3.763m ± 8%  ~ (p=0.310 n=6)   3.506m ± 9%  ~ (p=0.180 n=6)

                            │   main.txt   │       withoutState.txt        │         syncpool.txt          │
                            │     B/op     │     B/op      vs base         │     B/op      vs base         │
PushMetricsVaryingMetrics-2   2.335Mi ± 0%   2.335Mi ± 0%  ~ (p=0.619 n=6)   2.335Mi ± 0%  ~ (p=0.589 n=6)

                            │  main.txt   │       withoutState.txt       │         syncpool.txt         │
                            │  allocs/op  │  allocs/op   vs base         │  allocs/op   vs base         │
PushMetricsVaryingMetrics-2   27.93k ± 0%   27.92k ± 0%  ~ (p=0.675 n=6)   27.92k ± 0%  ~ (p=0.729 n=6)
Here are the individual benchmark runs

Main:

arthursens$ export bench=main && go test -benchmem -benchtime 10s -count 6 -cpu 2 -timeout 999m -run=^$ -bench ^BenchmarkPushMetricsVaryingMetrics$ github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter | tee ${bench}.txt

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter
cpu: Apple M2 Pro
BenchmarkPushMetricsVaryingMetrics-2   	    3909	   3427968 ns/op	 2448636 B/op	   27926 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3986	   3634201 ns/op	 2447842 B/op	   27919 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3805	   3672838 ns/op	 2448664 B/op	   27926 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3895	   3749442 ns/op	 2447577 B/op	   27915 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3969	   3528172 ns/op	 2448437 B/op	   27926 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3769	   3800376 ns/op	 2448643 B/op	   27926 allocs/op
PASS
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter	117.336s

This branch:

arthursens$ export bench=withoutState && go test -benchmem -benchtime 10s -count 6 -cpu 2 -timeout 999m -run=^$ -bench ^BenchmarkPushMetricsVaryingMetrics$ github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter | tee ${bench}.txt

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter
cpu: Apple M2 Pro
BenchmarkPushMetricsVaryingMetrics-2   	    3906	   3471643 ns/op	 2447831 B/op	   27919 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3771	   3986271 ns/op	 2447340 B/op	   27914 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3409	   3762875 ns/op	 2448431 B/op	   27925 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3781	   3762294 ns/op	 2448643 B/op	   27926 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3740	   3992029 ns/op	 2449503 B/op	   27933 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3766	   3606582 ns/op	 2447805 B/op	   27919 allocs/op
PASS
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter	116.507s

Alternative 2 (#36601):

arthursens$ export bench=syncpool && go test -benchmem -benchtime 10s -count 6 -cpu 2 -timeout 999m -run=^$ -bench ^BenchmarkPushMetricsVaryingMetrics$ github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter | tee ${bench}.txt

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter
cpu: Apple M2 Pro
BenchmarkPushMetricsVaryingMetrics-2   	    3654	   3329788 ns/op	 2447813 B/op	   27919 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3249	   3200849 ns/op	 2448696 B/op	   27926 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3835	   3419048 ns/op	 2447588 B/op	   27915 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3921	   3660168 ns/op	 2448859 B/op	   27927 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3769	   3592245 ns/op	 2448690 B/op	   27927 allocs/op
BenchmarkPushMetricsVaryingMetrics-2   	    3870	   3678666 ns/op	 2447850 B/op	   27920 allocs/op
PASS
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/prometheusremotewriteexporter	122.461s

Signed-off-by: Arthur Silva Sens <[email protected]>
@dashpole
Copy link
Contributor

dashpole commented Dec 2, 2024

cc @ben-childs-docusign
Can you help us reproduce the performance improvements from #34271 in a benchmark?

@ArthurSens
Copy link
Member Author

Closing in favor of #36601 (comment)

@ArthurSens ArthurSens closed this Dec 13, 2024
@ArthurSens ArthurSens deleted the prwexporter-remove-batchstate branch December 13, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants