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

[processor/deltatocumulativeprocessor] Adjust equal test to not compare functions #38008

Merged
merged 9 commits into from
Feb 20, 2025

Conversation

mauri870
Copy link
Contributor

@mauri870 mauri870 commented Feb 18, 2025

Description

With the collector now on Go 1.23 I'm working on adding iterator support for pdata at open-telemetry/opentelemetry-collector#12380 but the EqualT_test fails when comparing the iterators returned by All.

Since the reflect value of an iter.Seq/iter.Seq2 is reflect.Func, they are not comparable in the usual sense. The go-cmp/cmp is still on Go 1.21 so it does not know about iterators. There is x/exp/xiter.Equal but it is still an open proposal.
See https://go.googlesource.com/go/+/81c66e71d480ae2372b7eea4bcdf600b50fdd5e1/src/reflect/deepequal.go#158.

Examples of the test failures:

expo_test.go:203: Attributes().All(): 0xd1b400 != 0xd1b400
expo_test.go:203: Exemplars().All(): 0xe2be20 != 0xe2be20

This PR adjusts the equal logic to skip the equality check for functions.

Link to tracking issue

For open-telemetry/opentelemetry-collector#12380

@mauri870 mauri870 marked this pull request as draft February 18, 2025 12:48
@mauri870
Copy link
Contributor Author

I don't think this needs a changelog entry so I appreciate if a mantainer can add the Skip Changelog label or equivalent.

Copy link
Member

@sh0rez sh0rez left a comment

Choose a reason for hiding this comment

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

Oh interesting. Looking at this, I think there is little value in comparing returned funcs at all (whether they are iterators or not). how about we skip them altogether?

@mauri870
Copy link
Contributor Author

how about we skip them altogether?

Fair point, I will do that.

@mauri870 mauri870 changed the title [processor/deltatocumulativeprocessor] Adjust equal test to not compare iterators [processor/deltatocumulativeprocessor] Adjust equal test to not compare functions Feb 18, 2025
@dehaansa dehaansa added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 18, 2025
@mauri870 mauri870 requested a review from sh0rez February 18, 2025 16:20
@sh0rez
Copy link
Member

sh0rez commented Feb 19, 2025

@open-telemetry/collector-contrib-approvers this is ready to merge

@dehaansa dehaansa added the ready to merge Code review completed; ready to merge by maintainers label Feb 19, 2025
@mx-psi mx-psi merged commit d09a0cc into open-telemetry:main Feb 20, 2025
173 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/deltatocumulative ready to merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants