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 ForEach methods to reduce deeply nested looping #8927

Closed
djaglowski opened this issue Nov 15, 2023 · 4 comments
Closed

[pdata] Add ForEach methods to reduce deeply nested looping #8927

djaglowski opened this issue Nov 15, 2023 · 4 comments
Labels
area:pdata pdata module related issues enhancement New feature or request

Comments

@djaglowski
Copy link
Member

Is your feature request related to a problem? Please describe.
Drilling through pdata structs in order to perform work on each items is very common but generally requires deeply nested and repetitive code.

func doSomethingWithLogs(ld plog.Logs) {
	rls := ld.ResourceLogs()
	for i := 0; i < rls.Len(); i++ {
		rl := rls.At(i)
		sls := rl.ScopeLogs()
		for j := 0; j < sls.Len(); j++ {
			sl := sls.At(j)
			for k := 0; k < sl.LogRecords().Len(); k++ {
				lr := sl.LogRecords().At(k)
				// do something with rl, sl, lr
			}
		}
	}
}

Describe the solution you'd like
We should consider adding ForEach* methods at various levels in the pdata hierarchy.

The same code above could look something like:

func doSomethingWithLogs(ld plog.Logs) {
	ld.ForEachLogRecord(func(rl plog.ResourceLogs, sl plog.ScopeLogs, lr plog.LogRecord) {
		// do something with rl, sl, lr
	})
}

This could be implemented at the level of each "Slice" struct. e.g.

func (es ResourceLogsSlice) ForEachResourceLogs(func(rl plog.ResourceLogs))
func (es ResourceLogsSlice) ForEachScopeLogs(func(rl plog.ResourceLogs, sl plog.ScopeLogs))
func (es ResourceLogsSlice) ForEachLogRecord(func(rl plog.ResourceLogs, sl plog.ScopeLogs, lr plog.LogRecord))

func (es ScopeLogsSlice) ForEachScopeLogs(func(sl plog.ScopeLogs))
func (es ScopeLogsSlice) ForEachLogRecord(func(sl plog.ScopeLogs, lr plog.LogRecord))

func (es LogRecordSlice) ForEachLogRecord(func(lr plog.LogRecord))

Optionally, we could go one step further and expose these on the structs which contain Slices. e.g.

func (ms Logs) ForEachResourceLogs(func(rl plog.ResourceLogs))
func (ms Logs) ForEachScopeLogs(func(rl plog.ResourceLogs, sl plog.ScopeLogs))
func (ms Logs) ForEachLogRecord(func(rl plog.ResourceLogs, sl plog.ScopeLogs, lr plog.LogRecord))

func (ms ScopeLogs) ForEachScopeLogs(func(sl plog.ScopeLogs))
func (ms ScopeLogs) ForEachLogRecord(func(sl plog.ScopeLogs, lr plog.LogRecord))

Describe alternatives you've considered
As I understand it, the current pattern of requiring iteration using Len and At was intentional based on slight advantage in performance. I believe this level of optimization is helpful in some cases and should remain on the table as an option. However, I believe there is a tradeoff with component writability and maintainability which should be considered here as well.

Additional context
I'm not certain this would work as nicely for metric data points, since we have multiple types. Perhaps someone has ideas or opinions on this aspect in particular. e.g.

func (es ResourceMetricsSlice) ForEachNumberDataPoint(rm pmetric.ResourceMetrics, sm pmetric.ScopeMetrics, m pmetric.Metric, dp pmetric.NumberDataPoint)
@djaglowski
Copy link
Member Author

cc @mx-psi @bogdandrutu

@bogdandrutu
Copy link
Member

Personal preference:

  1. Add for in every Slice in the pdata generator:
func (es ResourceLogsSlice) ForEach(func(rl plog.ResourceLogs))

func (es ScopeLogsSlice) ForEach(func(sl plog.ScopeLogs))

func (es LogRecordSlice) ForEach(func(lr plog.LogRecord))
  1. Add high-level ForEach in top level structs:
func (ms Logs) ForEachLogRecord(func(rl plog.ResourceLogs, sl plog.ScopeLogs, lr plog.LogRecord))
  1. Wait for more feedback to come.

@mx-psi mx-psi added enhancement New feature or request area:pdata pdata module related issues labels Nov 16, 2023
@mx-psi
Copy link
Member

mx-psi commented Nov 16, 2023

I think we can clearly do this in an additive way, so this is not required for pdata 1.0. I like @bogdandrutu's plan (but maybe we don't need all ForEachs, I suspect for the most part components will benefit from ForEachLogRecord and similar methods)

@djaglowski
Copy link
Member Author

Closing in favor of #11982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:pdata pdata module related issues enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants