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

[processor/deltatocumulative]: bugfix - permit advancing start-time #31365

Merged

Conversation

sh0rez
Copy link
Member

@sh0rez sh0rez commented Feb 21, 2024

Description:

The spec allows the StartTimestamp of a delta stream to advance over time, without affecting the aggregation.

Previously, the running counter was reset with each start-time change. This was violating the spec and also effectively prevented meaningful aggregation as series were reset all the time.

Testing:

A test-case was modified to explicitly check this behavior is permitted and handled correctly.

@sh0rez sh0rez force-pushed the deltatocumulative-advancing-starttime branch from 9893da1 to 9163f48 Compare February 21, 2024 12:24
@ethercrow
Copy link

Imagine this situation:

  1. Collector receives a data point saying 'Counter C has seen D1 increments in the interval from T1 to T2'
  2. Later it receives a data point saying `Counter C has seen D2 increments in the interval from T3 to T4'

And T1 < T2 < T3 < T4. The interval from T2 to T3 is unaccounted for and even if a corresponding data point arrives later it would be ignored. Or maybe it gets lost for some reason.

What do you think about a metric that tracks this unaccounted time? Since there are no more resets, it would replace the resets metric from #31363 it would record those skips like from T2 to T3 in the example above.

@sh0rez
Copy link
Member Author

sh0rez commented Feb 28, 2024

@ethercrow I like that, but I think it's out of scope for this PR (given the metrics one isn't even merged). But lets add it later 👍

@sh0rez
Copy link
Member Author

sh0rez commented Feb 28, 2024

@RichieSams @djaglowski please take a look!

@jpkrohling jpkrohling merged commit 298dda4 into open-telemetry:main Feb 28, 2024
142 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 28, 2024
@RichieSams
Copy link
Contributor

Ah, they beat me to it. Good find :)

@sh0rez sh0rez deleted the deltatocumulative-advancing-starttime branch February 28, 2024 17:22
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
…pen-telemetry#31365)

**Description:** <Describe what has changed.>

The
[spec](https://opentelemetry.io/docs/specs/otel/metrics/data-model/#sums-delta-to-cumulative)
allows the `StartTimestamp` of a delta stream to advance over time,
without affecting the aggregation.

Previously, the running counter was reset with each start-time change.
This was violating the spec and also effectively prevented meaningful
aggregation as series were reset all the time.

**Testing:**

A test-case was modified to explicitly check this behavior is permitted
and handled correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants