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

pdata: add iterator All method to slice and map types #12380

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mauri870
Copy link
Contributor

@mauri870 mauri870 commented Feb 13, 2025

Description

This PR adds support for iterators to Slice and Map types and their autogenerated counterparts. Iterators were stabilized in Go 1.23.

The All method is analogous to maps.All and slices.All and is a more idiomatic alternative to the more verbose for i := 0; i < es.Len(); i++ { z := es.At(i) } way of looping.

Code that is written like this today:

for i := 0; i < ld.ResourceLogs().Len(); i++ {
        rl := ld.ResourceLogs().At(i)
        for j := 0; j < rl.ScopeLogs().Len(); j++ {
                sl := rl.ScopeLogs().At(j)
                for k := 0; k < sl.LogRecords().Len(); k++ {
                        lr := sl.LogRecords().At(k)
                        // use lr
                }
        }
}

Can now be written like this:

for _, rl := range ld.ResourceLogs().All() {
        for _, sl := range rl.ScopeLogs().All() {
                for _, lr := range sl.LogRecords().All() {
                        // use lr
                }
        }
}

Link to tracking issue

Fixes #11982

@mauri870 mauri870 marked this pull request as ready for review February 13, 2025 11:49
@mauri870 mauri870 requested review from mx-psi, dmathieu and a team as code owners February 13, 2025 11:49
@mx-psi
Copy link
Member

mx-psi commented Feb 13, 2025

@djaglowski Do you think this would improve #8927 ? Is there a way we can make it solve #8927?

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 51.52838% with 111 lines in your changes missing coverage. Please review.

Project coverage is 91.82%. Comparing base (56fbf4c) to head (7112cb8).

Files with missing lines Patch % Lines
pdata/pcommon/generated_byteslice.go 50.00% 2 Missing and 1 partial ⚠️
pdata/pcommon/generated_float64slice.go 50.00% 2 Missing and 1 partial ⚠️
pdata/pcommon/generated_int32slice.go 50.00% 2 Missing and 1 partial ⚠️
pdata/pcommon/generated_int64slice.go 50.00% 2 Missing and 1 partial ⚠️
pdata/pcommon/generated_stringslice.go 50.00% 2 Missing and 1 partial ⚠️
pdata/pcommon/generated_uint64slice.go 50.00% 2 Missing and 1 partial ⚠️
pdata/pcommon/map.go 57.14% 2 Missing and 1 partial ⚠️
pdata/pcommon/slice.go 50.00% 2 Missing and 1 partial ⚠️
pdata/plog/generated_logrecordslice.go 50.00% 2 Missing and 1 partial ⚠️
pdata/plog/generated_resourcelogsslice.go 50.00% 2 Missing and 1 partial ⚠️
... and 27 more

❌ Your patch check has failed because the patch coverage (51.52%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12380      +/-   ##
==========================================
- Coverage   92.18%   91.82%   -0.37%     
==========================================
  Files         465      465              
  Lines       25275    25495     +220     
==========================================
+ Hits        23301    23410     +109     
- Misses       1575     1649      +74     
- Partials      399      436      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djaglowski
Copy link
Member

@djaglowski Do you think this would improve #8927 ? Is there a way we can make it solve #8927?

I've closed #8927 in favor of #11982. I think this solves the same problem.

@mauri870 mauri870 marked this pull request as draft February 13, 2025 14:17
@mauri870
Copy link
Contributor Author

mauri870 commented Feb 14, 2025

Any ideas on what could be causing the failures https://github.com/open-telemetry/opentelemetry-collector/actions/runs/13333923272/job/37244695282?pr=12380#step:4:3611? It looks like a check for the new All method is being automatically added in some contrib tests. Since Range isn't included there, there must be a way to bypass this—I just don't know what it is.

@mauri870
Copy link
Contributor Author

mauri870 commented Feb 18, 2025

The failures in contrib-tests-matrix is due to comparing iter.Seq2 against each other. Since iter.Seq is a non-nil reflect.Func the comparison will always return false. I opened a PR fixing the test at open-telemetry/opentelemetry-collector-contrib#38008.

I assume that once that PR is merged this test will pass.

mx-psi pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Feb 20, 2025
…re functions (#38008)

#### 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:

```txt
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pdata] Implement Go 1.23 style iterators
5 participants