[processor/interval] Implement the main logic #32054

RichieSams · 2024-04-01T14:04:51Z

Description:

This PR implements the main logic of this new processor.

Link to tracking Issue:
#29461
This is a re-opening of 30827

Testing:
I added a test for the main aggregation / export behavior, but I need to add more to test state expiry

Documentation:

I updated the README with the updated configs, but I should add some examples as well.

github-actions · 2024-04-16T05:19:50Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

RichieSams · 2024-04-18T12:17:43Z

@sh0rez @jpkrohling This PR is ready for review now.

Can you help me understand why the tests are failing? It says that the go.sum is missing a reference to github.com/open-telemetry/opentelemetry-collector-contrib/internal/exp/metrics

But in the go.mod I have:

replace github.com/open-telemetry/opentelemetry-collector-contrib/internal/exp/metrics => ../../internal/exp/metrics

Which matches all the other processors. Locally it compiles fine. So I'm a bit stumped...

RichieSams · 2024-04-18T15:07:15Z

I just discovered golden.ReadMetrics(filePath string) from another component. Which could really simplify my test code. I'll take a look at using that

RichieSams · 2024-04-22T12:01:31Z

@sh0rez @djaglowski @jpkrohling I have updated the tests, and this is ready for review again. (Though I'm still stumped why the CI tests are failing... )

sh0rez

It appears to me this processor can be simplified quite a lot (and be more efficient) by having aggregate create what export needs. Some pseudo-code of the idea:

keep the last datapoint per stream in memory. replace with other datapoint if has more recent .Timestamp().
store them in a pmetric.Metrics. Populate resource, scope and metric as needed from incoming sample. have some maps for lookups (they store only the pointer types, no actual data)
export is now trivial: return the already built pmetric.Metrics, clear the state

type State struct {
	raw pmetric.Metrics

	rms map[identity.Resource]pmetric.ResourceMetrics
	sms map[identity.Scope]pmetric.ScopeMetrics
	ms map[identity.Metric]pmetric.Metric

	nums map[identity.Stream]pmetric.NumberDataPoint
}

func (s State) Update(res Resource, scope Scope, m Metric) {
	lock()
	defer unlock()

	for _, dp := range md.DataPoints {
		id := identity.OfStream(dp)
		aggr := s.Number(id)
		if aggr.Ts < dp.Ts {
			dp.CopyTo(aggr)
		}
	}
}

func (s State) Number(id identity.Stream, res Resource, scope Scope, m Metric) pmetric.NumberDataPoint {
	// returns or creates the pmetric.NumberDataPoint in s.nums for given stream.
	// populates the required rms, sms and ms in raw as needed
}

func (s State) Export() pmetric.Metrics {
	lock()
	defer unlock()

	out := s.raw
	s.raw = pmetric.NewMetrics()
	clear(s.rms); clear(s.sms); clear(s.ms); clear(s.nums)

	return out
}

I think this could minimize memory usage and requires a lot less data conversion.

Does this make sense, or did I miss something important?

internal/exp/metrics/streams/streams.go

processor/intervalprocessor/README.md

processor/intervalprocessor/config.go

processor/intervalprocessor/internal/metrics/metrics.go

processor/intervalprocessor/processor.go

RichieSams · 2024-04-22T14:17:32Z

It appears to me this processor can be simplified quite a lot (and be more efficient) by having aggregate create what export needs

I like your idea! It would also solve the issue that NewMetric() should only be used in testing. I'll take a swing at it today

RichieSams · 2024-04-23T00:36:55Z

@sh0rez Updated. Ready for re-review as you're able :)

No need to copy as much data back and forth

sh0rez · 2024-04-24T15:34:00Z

@RichieSams any specific reason to force-push this PR? makes reviewing really hard, as recent changes can't be easily identified

processor/intervalprocessor/processor.go

sh0rez · 2024-04-24T15:36:38Z

processor/intervalprocessor/internal/metrics/metrics.go

+	"go.opentelemetry.io/collector/pdata/pmetric"
+)
+
+type DataPointSlice[DP DataPoint[DP]] interface {


these types are only locally used once in aggregateDataPoints, doesn't need an extra package, right?

Yes, that package has gotten much much smaller since the original design. That said, I'm working on a PR for another component, and needed the same interfaces. So I'm potentially going to put them in internal/exp/metrics/streams. See:

RichieSams@d16a215#diff-45d5f87d8148e341ebb68b4125ebe4296ccfbb1a9b63a45009358e7a0b617d26

Thoughts?

sh0rez · 2024-04-24T15:38:02Z

processor/intervalprocessor/processor.go

+}
+
+func (p *Processor) exportMetrics() {
+	md := func() pmetric.Metrics {


why the closure?

So the lock only lasts for the fetch, and not for the duration of ConsumeMetrics. Since defer is on function exit, not scope exit.

RichieSams · 2024-04-24T17:39:17Z

@sh0rez

any specific reason to force-push this PR? makes reviewing really hard, as recent changes can't be easily identified

Heh, a combination of habits. At work we're not allowed to have merge commits. So I generally always rebase and squash commits locally and force push. Github has little buttons to show the diff, but looking at it, it's not ideal, since it includes all the changes from main as well.

This project always squashes, so I'll do a merge commit next time to catch up to HEAD. Then you can see my changes as individual commits.

RichieSams · 2024-04-29T17:45:40Z

@djaglowski @evan-bradley @TylerHelmuth Any further questions or concerns?

RichieSams · 2024-04-29T18:48:29Z

Can someone re-kick the coverage task. It's failing due to rate-limiting:

info - 2024-04-29 18:27:29,491 -- The PR is happening in a forked repo. Using tokenless upload.
info - 2024-04-29 18:27:29,671 -- Process Commit creating complete
debug - 2024-04-29 18:27:29,671 -- Commit creating result --- {"result": "RequestResult(error=RequestError(code='HTTP Error 429', params={}, description='{\"detail\":\"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 418 seconds.\"}'), warnings=[], status_code=429, text='{\"detail\":\"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 418 seconds.\"}')"}
error - 2024-04-29 18:27:29,672 -- Commit creating failed: {"detail":"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 418 seconds."}
Error: Codecov: Failed to properly create commit: The process '/home/runner/work/_actions/Wandalen/codecov-action/dist/codecov' failed with exit code 1
Error: Process returned exit code 1
Launched as "/home/runner/runners/2.316.0/externals/node20/bin/node /home/runner/work/_actions/Wandalen/wretry.action/v3.4.0_js_action/src/Runner.mjs /home/runner/work/_actions/Wandalen/codecov-action/dist/index.js"
Launched at "/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib"
Attempts exhausted, made 10 attempts :
Attempt #1 started at : 18:24:56 GMT+0000 (Coordinated Universal Time)
Attempt #2 started at : 18:25:13 GMT+0000 (Coordinated Universal Time)
Attempt #3 started at : 18:25:30 GMT+0000 (Coordinated Universal Time)
Attempt #4 started at : 18:25:47 GMT+0000 (Coordinated Universal Time)
Attempt #5 started at : 18:26:04 GMT+0000 (Coordinated Universal Time)
Attempt #6 started at : 18:26:20 GMT+0000 (Coordinated Universal Time)
Attempt #7 started at : 18:26:37 GMT+0000 (Coordinated Universal Time)
Attempt #8 started at : 18:26:54 GMT+0000 (Coordinated Universal Time)
Attempt #9 started at : 18:27:11 GMT+0000 (Coordinated Universal Time)
Attempt #10 started at : 18:27:28 GMT+0000 (Coordinated Universal Time)

RichieSams · 2024-04-30T13:48:17Z

Is this ok to merge if I update to main once more?

Description: This PR implements the main logic of this new processor. Link to tracking Issue: open-telemetry#29461 This is a re-opening of [30827](open-telemetry#30827) Testing: I added a test for the main aggregation / export behavior, but I need to add more to test state expiry Documentation: I updated the README with the updated configs, but I should add some examples as well.

RichieSams requested a review from a team as a code owner April 1, 2024 14:04

RichieSams requested a review from dashpole April 1, 2024 14:04

github-actions bot assigned TylerHelmuth Apr 1, 2024

github-actions bot added internal/exp/metrics processor/deltatocumulative processor/interval labels Apr 1, 2024

github-actions bot requested a review from sh0rez April 1, 2024 14:05

RichieSams marked this pull request as draft April 1, 2024 14:07

github-actions bot added the cmd/otelcontribcol otelcontribcol command label Apr 1, 2024

github-actions bot added the Stale label Apr 16, 2024

RichieSams force-pushed the processor/interval branch 2 times, most recently from dd599e7 to a902abc Compare April 17, 2024 21:07

github-actions bot removed the Stale label Apr 18, 2024

RichieSams force-pushed the processor/interval branch from a902abc to 11c4a6d Compare April 18, 2024 11:49

RichieSams marked this pull request as ready for review April 18, 2024 12:16

github-actions bot assigned evan-bradley Apr 18, 2024

TylerHelmuth assigned djaglowski Apr 18, 2024

sh0rez suggested changes Apr 22, 2024

View reviewed changes

RichieSams force-pushed the processor/interval branch from ddcee21 to bd53d3f Compare April 23, 2024 00:36

RichieSams force-pushed the processor/interval branch 4 times, most recently from 6de69ee to 920a778 Compare April 23, 2024 19:47

RichieSams requested a review from sh0rez April 24, 2024 12:36

RichieSams added 12 commits April 24, 2024 08:43

Add a changelog entry

9113d2f

Put exportMetricsLoop inline

72fdba1

Simplify the processor logic

e40655b

No need to copy as much data back and forth

Revert

d726e44

Fix dependencies

aee70ef

Fix linting

238cec3

One more fix

540bca7

Run gci

b5124d3

Fix crosslink

8d3cae8

make tidy

b7e4d35

Make generate

983bab3

Fix generate

03a3f79

RichieSams force-pushed the processor/interval branch from c547623 to 03a3f79 Compare April 24, 2024 12:44

sh0rez approved these changes Apr 24, 2024

View reviewed changes

RichieSams added 4 commits April 24, 2024 13:47

Merge branch 'main' into processor/interval

e2c2d1d

Use a local variable for the export ticker

375c3c5

Small comment clarification

b5bf916

Merge branch 'main' into processor/interval

08282c8

Merge branch 'main' into processor/interval

6e3601a

djaglowski approved these changes Apr 30, 2024

View reviewed changes

djaglowski merged commit 4f96a92 into open-telemetry:main Apr 30, 2024
170 checks passed

github-actions bot added this to the next release milestone Apr 30, 2024

RichieSams deleted the processor/interval branch April 30, 2024 14:53

RichieSams mentioned this pull request May 1, 2024

Aggregate metric datapoints over time period #29461

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[processor/interval] Implement the main logic #32054

[processor/interval] Implement the main logic #32054

RichieSams commented Apr 1, 2024 •

edited

Loading

github-actions bot commented Apr 16, 2024

RichieSams commented Apr 18, 2024

RichieSams commented Apr 18, 2024 •

edited

Loading

RichieSams commented Apr 22, 2024

sh0rez left a comment •

edited

Loading

RichieSams commented Apr 22, 2024

RichieSams commented Apr 23, 2024

sh0rez commented Apr 24, 2024

sh0rez Apr 24, 2024

RichieSams Apr 24, 2024

sh0rez Apr 24, 2024

RichieSams Apr 24, 2024

RichieSams commented Apr 24, 2024 •

edited

Loading

RichieSams commented Apr 29, 2024

RichieSams commented Apr 29, 2024

RichieSams commented Apr 30, 2024

[processor/interval] Implement the main logic #32054

[processor/interval] Implement the main logic #32054

Conversation

RichieSams commented Apr 1, 2024 • edited Loading

github-actions bot commented Apr 16, 2024

RichieSams commented Apr 18, 2024

RichieSams commented Apr 18, 2024 • edited Loading

RichieSams commented Apr 22, 2024

sh0rez left a comment • edited Loading

Choose a reason for hiding this comment

RichieSams commented Apr 22, 2024

RichieSams commented Apr 23, 2024

sh0rez commented Apr 24, 2024

sh0rez Apr 24, 2024

Choose a reason for hiding this comment

RichieSams Apr 24, 2024

Choose a reason for hiding this comment

sh0rez Apr 24, 2024

Choose a reason for hiding this comment

RichieSams Apr 24, 2024

Choose a reason for hiding this comment

RichieSams commented Apr 24, 2024 • edited Loading

RichieSams commented Apr 29, 2024

RichieSams commented Apr 29, 2024

RichieSams commented Apr 30, 2024

RichieSams commented Apr 1, 2024 •

edited

Loading

RichieSams commented Apr 18, 2024 •

edited

Loading

sh0rez left a comment •

edited

Loading

RichieSams commented Apr 24, 2024 •

edited

Loading