receiver/prometheusreceiver: allow cumulative resets when using the adjuster #37718
Conversation
…djuster Fixes open-telemetry#37717 Prior to this change, when the start time metric adjuster was used all points used the same start timestamp. Even after a reset, which makes no sense for a counter which is not supposed to go down. Instead this change makes it so that when a reset is detected, the the reset points timestamp is used as the next start time. Signed-off-by: Ridwan Sharif <[email protected]>
ridwanmsharif
force-pushed
the
ridwanmsharif/metricadjuster-reset-v2
branch
from
6b0261b to
c5fd0ed
Compare
| // point instead of the start timestamp when it detects resets. This is | ||
| // useful when this adjuster is used after another adjuster that | ||
| // pre-populated start times. | ||
| usePointTimeForReset bool |
There was a problem hiding this comment.
why is this a configuration option? Seems like it is always true?
There was a problem hiding this comment.
Its not set to true when the adjuster is set up directly (using the NewInitialPointMetricAdjuster) We only set it to true when this adjuster is used as part of the StartTimeMetricAdjuster.
There was a problem hiding this comment.
Should this be true also for NewInitialPointAdjuster? If a value decreases, it seems like we should always update the start time...
There was a problem hiding this comment.
Sorry I'll clarify, this knob just determines what value to use when updating the start time when a reset is detected (the start time of the reset point, or the point time of the reset point).
The InitialPointAdjuster correctly always updates the start time when a reset is detected. The issue is that it uses the .StartTimestamp() of the reset point instead of the .Timestamp(), but this is actually not what we want when this adjuster is used as part of the StartTimeMetricAdjuster.
This is cause the StartTimeMetricAdjuster first loops over all metrics and updates all start times to what it found in process_start_time_seconds (or the fallback) before it applies the InitialMetricAdjuster. Thats why, we use the point timestamp of the reset point instead of the start timestamp (which would be incorrect) in this case where the InitialPointAdjuster is used as part of the StartTimeMetricAdjuster.
I see 2 ways we can get rid of the config knob I added:
- Always use the reset points timestamp instead of
.StartTimestamp()when a new reset point is detected. This is usually true but there maybe cases where a reset point will have a start time != point time. - Duplicate the code of the
InitialPointAdjusterthat is dealing with the resets, and add it inline in theStartTimeMetricAdjusterbut I think this is worse than what we have now.
dashpole
added
ready to merge
* main: (76 commits) Update All OpenTelemetry Collector Contrib packages (open-telemetry#37839) [chore] fix codeowners allowlist (open-telemetry#37856) Update module github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp to v1.26.0 (open-telemetry#37841) Update module github.com/tencentcloud/tencentcloud-sdk-go/tencentcloud/common to v1.0.1095 (open-telemetry#37825) Update module github.com/grpc-ecosystem/grpc-gateway/v2 to v2.26.1 (open-telemetry#37821) [chore][exporter/elasticsearch] Bump go-docappender to v2.5.0 (open-telemetry#37852) Update All github.com/aws packages (open-telemetry#37816) Update module google.golang.org/protobuf to v1.36.5 (open-telemetry#37827) Update module github.com/SAP/go-hdb to v1.12.12 (open-telemetry#37817) Update module github.com/huaweicloud/huaweicloud-sdk-go-v3 to v0.1.135 (open-telemetry#37822) Update module github.com/ClickHouse/clickhouse-go/v2 to v2.31.0 (open-telemetry#37835) [receiver/datadog] Implement support for span links (open-telemetry#37449) receiver/prometheusreceiver: allow cumulative resets when using the adjuster (open-telemetry#37718) Update README.md (open-telemetry#37826) [receiver/github] add workflow run event trace handling (open-telemetry#37578) Update All github.com/datadog packages to v0.62.2 (open-telemetry#37838) [chore] Update types used in extensiontest.NewNopSettingsWithType (open-telemetry#37844) [processor/redaction] introduce `allowed_values` parameter in processor config (open-telemetry#37638) [chore] Update otel version (open-telemetry#37808) [testbed] Include CPU and memory limits to benchmark results file (open-telemetry#36753) ...
…djuster (open-telemetry#37718) Fixes open-telemetry#37717 Prior to this change, when the start time metric adjuster was used all points used the same start timestamp. Even after a reset, which makes no sense for a counter which is not supposed to go down. Instead this change makes it so that when a reset is detected, the the reset points timestamp is used as the next start time. Signed-off-by: Ridwan Sharif <[email protected]>
Fixes #37717
Prior to this change, when the start time metric adjuster was used all points used the same start timestamp. Even after a reset, which makes no sense for a counter which is not supposed to go down.
Instead this change makes it so that when a reset is detected, the the reset points timestamp is used as the next start time.