Skip to content

Conversation

@bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Aug 28, 2025

This happens because the proto message is generated with gogoproto, and it will go on the path https://github.com/gogo/protobuf/blob/f67b8970b736e53dbd7d0a27146c8f1ac52f74e5/proto/table_marshal.go#L2977

Benchmark numbers show improvement as well.

After:

BenchmarkExecute/numSample=100
BenchmarkExecute/numSample=100-12         	   21752	     52500 ns/op	    6635 B/op	      83 allocs/op
BenchmarkExecute/numSample=1000
BenchmarkExecute/numSample=1000-12        	   10000	    131918 ns/op	   23096 B/op	      86 allocs/op
BenchmarkExecute/numSample=10000
BenchmarkExecute/numSample=10000-12       	    1479	    886487 ns/op	   41477 B/op	      86 allocs/op

Before:

BenchmarkExecute/numSample=100
BenchmarkExecute/numSample=100-12         	   20642	     55670 ns/op	    6635 B/op	      83 allocs/op
BenchmarkExecute/numSample=1000
BenchmarkExecute/numSample=1000-12        	    8640	    145875 ns/op	   23121 B/op	      86 allocs/op
BenchmarkExecute/numSample=10000
BenchmarkExecute/numSample=10000-12       	    1015	   1257688 ns/op	  304366 B/op	      87 allocs/op

Copy link
Contributor

@jmichalek132 jmichalek132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@atoulme atoulme merged commit 1b12108 into open-telemetry:main Sep 2, 2025
306 of 309 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 2, 2025
@mx-psi
Copy link
Member

mx-psi commented Sep 4, 2025

This seems like the most likely cause of #42414. Should we revert until we can fix that?

@jmichalek132
Copy link
Contributor

This seems like the most likely cause of #42414. Should we revert until we can fix that?

I dropped your question into slack to get feedback from the maintainers of the prom components.

@jmichalek132
Copy link
Contributor

This seems like the most likely cause of #42414. Should we revert until we can fix that?

I think that's a good idea but, I am not an approver for the component so maybe you might want to wait for a reply from them?

cc @ywwg @krajorama @ArthurSens @dashpole

@ArthurSens
Copy link
Member

I'm sorry, but I couldn't find the time to review this. If you feel like reverting this one would solve the flaky tests, please go for it. If we notice that the flakiness is still ongoing after the revert, we can also revert the revert.

In an ideal world, merging PRs without approval from codeowners shouldn't happen. However, I feel like we have more PRs being opened than our current group of codeowners can handle, so I don't want to block PRs from collector maintainers because of our slow responsiveness.

I'm open to ideas here

@jmichalek132
Copy link
Contributor

fyi raised the PR to revert it #42501

@bogdandrutu bogdandrutu deleted the fix-proto-copy branch September 4, 2025 21:38
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.

6 participants