metricstarttimeprocessor: Implementation of the subtractinitial strategy #38594

ridwanmsharif · 2025-03-13T04:54:43Z

Description

Addresses #38379.

Some of this logic already existed in the GCM exporter.

For each resource, and for each timeseries

Skip if the point is not a cumulative sum, histogram, exp histogram, or summary.
Lookup the timeseries in the cache. If it is not present, drop this point from the batch, and store the timestamp and value (including bucket counts, etc) in the cache.
Subtract cumulative values (e.g. sum, bucket counts) of the cached timeseries from the current timeseries, and set the start timestamp to the cached point's timestamp.

Link to tracking issue

Fixes #38379.

Testing

Added unit and integration tests.

Documentation

Readme already covers this strategy

ridwanmsharif · 2025-03-13T04:55:56Z

This builds on top of #38583, which in turn builds on top of #38579. The PRs should be merged in reverse order.

ridwanmsharif · 2025-03-19T04:17:14Z

Note to the reviewers:

processor/metricstarttimeprocessor/internal/subtractinitial/adjuster.go contains all the logic of the new adjuster
processor/metricstarttimeprocessor/internal/subtractinitial/adjuster_test.go

The other changes were made so there is deduplication of the test runners (pulled out to a common testhelper internal module)

ridwanmsharif · 2025-03-19T14:11:00Z

cc @aabmass for context with the GCM exporter normalization this mimics

aabmass · 2025-03-19T14:55:31Z

fyi @pintohutch

atoulme · 2025-03-19T18:30:21Z

Given that the author is codeowner, who should review this change? What type of review would you like? Please indicate this so we can best help.

ridwanmsharif · 2025-03-19T19:20:28Z

Given that the author is codeowner, who should review this change? What type of review would you like? Please indicate this so we can best help.

@dashpole (the proposer and other codeowner) would be the ideal candidate, but in his stead, the owners of the googlecloud exporter that this change mimics, would suffice for a thorough review of the adjustment logic. Either of @pintohutch, @aabmass or @psx95 would be okay for this.

Will also need someone from open-telemetry/collector-contrib-approvers to then review to sanity check and merge once ready

pintohutch · 2025-03-24T14:15:08Z

Also @bwplotka FYI

pintohutch

Mostly questions - thanks for doing this!

processor/metricstarttimeprocessor/internal/subtractinitial/adjuster.go

processor/metricstarttimeprocessor/internal/subtractinitial/adjuster_test.go

processor/metricstarttimeprocessor/internal/subtractinitial/adjuster.go

processor/metricstarttimeprocessor/internal/subtractinitial/adjuster_test.go

github-actions · 2025-04-08T05:21:04Z

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

braydonk

Mostly naming nits, functionality LGTM

Makefile

processor/metricstarttimeprocessor/internal/testhelper/util.go

processor/metricstarttimeprocessor/internal/subtractinitial/adjuster.go

This change addresses open-telemetry#38379.

This change pulls out some common testing utils in a shared internal library. Signed-off-by: Ridwan Sharif <[email protected]>

… than point time

…iately after the reference is cached Signed-off-by: Ridwan Sharif <[email protected]>

ridwanmsharif · 2025-05-05T14:43:13Z

Also I'm curious why this strategy is called substractinitial not actually true reset given the definition of the latter matches this PR algo https://opentelemetry.io/docs/specs/otel/metrics/data-model/#cumulative-streams-inserting-true-reset-points 🙈 The current true reset strategy for metricstarttime is objectively broken.

Especially given this is a new process, why we cannot actually "fix" true reset and deprecate the adjuster in prometheusreceiver? Are we not confusing users with the current proposal?

Naming is hard 🙈

I think the existing strategy is also a valid one. Described in more detail here: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/metricstarttimeprocessor#strategy-true-reset-point
And its the default strategy used by the prometheus receiver (which this processor replaces).

Opting to keep things as is here to keep the PR ready but open to deprecating other strategies or simplifying this moving forward, should only this strategy be useful

braydonk · 2025-05-05T17:52:54Z

I think this is ready to merge since the other codeowner, dashpole, is on leave meaning it's just Ridwan. Bartek's most recent comments don't seem to be PR blocking based on my reading. A maintainer may remove this label if they think it's not ready to merge for whatever reason.

atoulme · 2025-05-05T17:54:50Z

I have reviewed the code summarily and it passes my review. Codeowners have reviewed and approved the change.

- **[receiver/prometheus] Upgrade receiver.prometheus.removeLegacyResourceAttributes feature flag (#39803)** - **Feature/update library versions (#39848)** - **metricstarttimeprocessor: Implementation of the subtractinitial strategy (#38594)** - **[mongodbatlasreceiver] Add baseURL config (#39345)** - **[receiver/splunkenterprise] Unexport InfoEntry and InfoContent (#39830)** - **[pkg/ottl] Add support for HasPrefix and HasSuffix (#39825)** - **[receiver/snowflakereceiver] fix(security): CVE-2025-46327 : Upgraded gosnowflake to v1.13.3 (#39862)** - **[pkg/winperfcounters] Add support to retrieve raw values (#39835)** - **Update All github.com/aws packages (#39867)** - **use a single version**  #### Description  #### Link to tracking issue Fixes  #### Testing  #### Documentation  --------- Signed-off-by: Ridwan Sharif <[email protected]> Signed-off-by: Bogdan Drutu <[email protected]> Co-authored-by: Curtis Robert <[email protected]> Co-authored-by: Ramachandran A G <[email protected]> Co-authored-by: Antoine Toulme <[email protected]> Co-authored-by: Ridwan Sharif <[email protected]> Co-authored-by: Artur Santos <[email protected]> Co-authored-by: Christos Markou <[email protected]> Co-authored-by: Edmo Vamerlatti Costa <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Bogdan Drutu <[email protected]> Co-authored-by: Paulo Janotti <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: opentelemetrybot <[email protected]>

…egy (open-telemetry#38594)  #### Description Addresses open-telemetry#38379. Some of this logic already existed in the [GCM exporter](https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/main/exporter/collector/internal/normalization/standard_normalizer.go). For each resource, and for each timeseries - Skip if the point is not a cumulative sum, histogram, exp histogram, or summary. - Lookup the timeseries in the cache. If it is not present, drop this point from the batch, and store the timestamp and value (including bucket counts, etc) in the cache. - Subtract cumulative values (e.g. sum, bucket counts) of the cached timeseries from the current timeseries, and set the start timestamp to the cached point's timestamp.  #### Link to tracking issue Fixes open-telemetry#38379.  #### Testing Added unit and integration tests.  #### Documentation Readme already covers this strategy  --------- Signed-off-by: Ridwan Sharif <[email protected]>

github-actions bot added the processor/metricstarttime label Mar 13, 2025

github-actions bot requested a review from dashpole March 13, 2025 04:55

ridwanmsharif mentioned this pull request Mar 13, 2025

[wip] metricstarttimeprocessor: add subtractinitial strategy implementation #38570

Closed

ridwanmsharif force-pushed the ridwanmsharif/subtract-strategy-v2 branch 4 times, most recently from 6f1f6f0 to 7c597ac Compare March 19, 2025 02:05

ridwanmsharif marked this pull request as ready for review March 19, 2025 02:05

ridwanmsharif requested a review from a team as a code owner March 19, 2025 02:05

github-actions bot assigned edmocosta Mar 19, 2025

ridwanmsharif force-pushed the ridwanmsharif/subtract-strategy-v2 branch 3 times, most recently from 5c61b16 to 51576dd Compare March 19, 2025 04:12

ridwanmsharif force-pushed the ridwanmsharif/subtract-strategy-v2 branch from 51576dd to 740b90a Compare March 19, 2025 05:39

atoulme added the waiting-for-code-owners label Mar 20, 2025

This was referenced Mar 23, 2025

Weekly Report: 2025-03-16 - 2025-03-23 LucaLanziani/opentelemetry-collector-contrib#16

Closed

Weekly Report: 2025-03-16 - 2025-03-23 LucaLanziani/opentelemetry-collector-contrib#17

Closed

pintohutch reviewed Mar 24, 2025

View reviewed changes

github-actions bot mentioned this pull request Mar 25, 2025

Weekly Report: 2025-03-18 - 2025-03-25 #38935

Closed

This was referenced Apr 1, 2025

Weekly Report: 2025-03-25 - 2025-04-01 #39070

Closed

Weekly Report: 2025-04-01 - 2025-04-08 #39228

Closed

github-actions bot added the Stale label Apr 8, 2025

ridwanmsharif force-pushed the ridwanmsharif/subtract-strategy-v2 branch from 0b562f5 to 7dd5402 Compare May 2, 2025 13:57

ridwanmsharif marked this pull request as ready for review May 2, 2025 13:58

github-actions bot assigned ArthurSens May 2, 2025

ridwanmsharif force-pushed the ridwanmsharif/subtract-strategy-v2 branch 4 times, most recently from 3004e15 to 736b2d3 Compare May 2, 2025 18:28

braydonk reviewed May 2, 2025

View reviewed changes

ridwanmsharif added 5 commits May 5, 2025 03:31

metricstarttimeprocessor: add subtractinitial strategy implementation

d558917

This change addresses open-telemetry#38379.

metricstarttimeprocessor: deduplicate testing helper methods

4cec659

This change pulls out some common testing utils in a shared internal library. Signed-off-by: Ridwan Sharif <[email protected]>

subtractinitial: clean up comments and helper definitions

b2026bb

metricstarttimeprocessor: reset detection sets start time to 1ms less…

3542f37

… than point time

metricstarttimeprocessor: address edge case where reset happens immed…

98f5481

…iately after the reference is cached Signed-off-by: Ridwan Sharif <[email protected]>

ridwanmsharif force-pushed the ridwanmsharif/subtract-strategy-v2 branch from 736b2d3 to 98f5481 Compare May 5, 2025 03:53

ridwanmsharif requested a review from braydonk May 5, 2025 03:53

braydonk approved these changes May 5, 2025

View reviewed changes

braydonk added ready to merge Code review completed; ready to merge by maintainers and removed waiting-for-code-owners labels May 5, 2025

atoulme merged commit d23e351 into open-telemetry:main May 5, 2025
189 of 190 checks passed

github-actions bot added this to the next release milestone May 5, 2025

ridwanmsharif deleted the ridwanmsharif/subtract-strategy-v2 branch May 5, 2025 17:59

mx-psi mentioned this pull request May 6, 2025

Fix update-otel #39876

Merged

mx-psi mentioned this pull request May 6, 2025

mx psi/merge attempt three #39880

Merged

metricstarttimeprocessor: Implementation of the subtractinitial strategy #38594

metricstarttimeprocessor: Implementation of the subtractinitial strategy #38594

Uh oh!

Conversation

ridwanmsharif commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Link to tracking issue

Testing

Documentation

Uh oh!

ridwanmsharif commented Mar 13, 2025

Uh oh!

ridwanmsharif commented Mar 19, 2025

Uh oh!

ridwanmsharif commented Mar 19, 2025

Uh oh!

aabmass commented Mar 19, 2025

Uh oh!

atoulme commented Mar 19, 2025

Uh oh!

ridwanmsharif commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pintohutch commented Mar 24, 2025

Uh oh!

pintohutch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 8, 2025

Uh oh!

braydonk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ridwanmsharif commented May 5, 2025

Uh oh!

braydonk commented May 5, 2025

Uh oh!

atoulme commented May 5, 2025

Uh oh!

Uh oh!

Uh oh!

ridwanmsharif commented Mar 13, 2025 •

edited

Loading

ridwanmsharif commented Mar 19, 2025 •

edited

Loading