Add Range methods to pdata slices #8938
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8938 +/- ##
==========================================
+ Coverage 91.50% 91.53% +0.03%
==========================================
Files 320 320
Lines 17189 17229 +40
==========================================
+ Hits 15728 15771 +43
+ Misses 1163 1161 -2
+ Partials 298 297 -1 ☔ View full report in Codecov by Sentry. |
|
@bogdandrutu, @mx-psi, I'm curious your thoughts on this, both for the |
| @@ -89,8 +89,7 @@ func (b *dataBuffer) logMetricDataPoints(m pmetric.Metric) { | |||
| } | |||
|
|
|||
| func (b *dataBuffer) logNumberDataPoints(ps pmetric.NumberDataPointSlice) { | |||
| for i := 0; i < ps.Len(); i++ { | |||
| p := ps.At(i) | |||
| ps.ForEachIndex(func(i int, p pmetric.NumberDataPoint) { | |||
There was a problem hiding this comment.
Not sure I like the "Index" suffix.
There was a problem hiding this comment.
Just the name, or the entire function?
If just the name, some alternatives:
ForEachWithIndex(not just for each index)Range(perhaps implying similarity to gorange'sindex, itempattern, which is what I was trying to emulate here)
Will try to think of other options..
There was a problem hiding this comment.
Alternately, we could just consolidate on this signature for ForEach and expect often the index is ignored.
There was a problem hiding this comment.
I would just drop the Index suffix. I am also wondering if we can just have a single ForEach method that is just the ForEachWhile one (although that means a bit more verbosity with an extra return true)
There was a problem hiding this comment.
I like Range to be have the same behavior as for range for maps/slices. So if we need the index, let's use Range, and for non-index version we can use ForEach.
There was a problem hiding this comment.
Thanks for testing this! I can see how "Range with a return value" does not lead to a good UX. An alternative option to adding Range[If/While/Until] is only adding Range and using the for-loop style for these short-circuiting cases. So my question is: Is just having Range and using the current for-loop style for the short-circuiting cases good enough? If not, why not? Is mixing styles too bad or hard for contributors writing code using pdata?
There was a problem hiding this comment.
The main reason I felt so motivated to find a solution for Range[If/While/Until] is that I found the short-circuit use case to be surprisingly common - about as common as Range. And, there are also quite a few cases where they are nested within each other.
So, while I think users could certainly mix Range and for-loops as necessary, I think it complicates the decision about which style to use. If we have both Range and Range[If/While/Until], I see it as a straightforward tradeoff between performance and read/writeability. If we omit the latter function, then the author has to first consider whether or not Range is functionally sufficient. Ultimately, I think it will feel like an incomplete API and result in a lot of "hmm, guess I need to rewrite this".
That said, I think that adding only Range is still a clear benefit and we could revisit Range[If/While/Until] if prompted by feedback.
There was a problem hiding this comment.
That said, I think that adding only Range is still a clear benefit and we could revisit Range[If/While/Until] if prompted by feedback.
If other @open-telemetry/collector-maintainers are okay with this and accept that we will add Range[If/While/Until] in the future if need be, I would like to do it this way.
There was a problem hiding this comment.
I still believe that "Range with a return value" is the right API for use, because it is the golang standard way (see https://pkg.go.dev/sync#Map.Range) to deal with this, and users should be familiar with that API.
There was a problem hiding this comment.
We have that kind of range with our own pcommon.Map https://pkg.go.dev/go.opentelemetry.io/collector/pdata/pcommon#Map.Range. It's unclear to me if this is the Golang standard just for maps or generally ranging over a data structure.
djaglowski
force-pushed
the
pdata-slice-foreach
branch
from
4d655a0 to
574442b
Compare
djaglowski
changed the title
djaglowski
force-pushed
the
pdata-slice-foreach
branch
from
574442b to
4d77ed5
Compare
djaglowski
force-pushed
the
pdata-slice-foreach
branch
from
4d77ed5 to
91fe031
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
@bogdandrutu how can we unblock this? |
Description:
This PR adds a
Rangemethod to each pdata slice struct.I updated many loops throughout the codebase to demonstrate usage of the functions. I may have missed a few or applied the pattern unnecessarily in simple situations, but primarily am seeking feedback on the implementation at this point.
Link to tracking Issue: #8927.
Testing:
Tests are added for generated methods.