Skip to content
This repository was archived by the owner on Dec 3, 2024. It is now read-only.

Record counters as CUMULATIVE metrics #24

Merged
merged 3 commits into from
Jan 6, 2023
Merged

Record counters as CUMULATIVE metrics #24

merged 3 commits into from
Jan 6, 2023

Conversation

n-oden
Copy link
Contributor

@n-oden n-oden commented Dec 9, 2022

This addresses #18

While using |align delta_gauge() in MQL is potentially a workaround, it suffers from the drawback of producing huge negative deltas when the counter resets, and the current behavior is unhappily surprising to first-time users who would reasonably assume that IncrCounter() would produce a CUMULATIVE metric.

Signed-off-by: Nathan J. Mehl n@oden.io

@n-oden
Copy link
Contributor Author

n-oden commented Dec 9, 2022

FWIW: I believe that this approach avoids the difficulties encountered in #19 -- the stackdriver API for CUMULATIVE metrics is extremely fussy about start/end times, and there's really no getting around needing to keep track of your own start time.

@n-oden
Copy link
Contributor Author

n-oden commented Dec 20, 2022

@tam7t thoughts? :) We've worked around this locally by copy-pasting a patched stackdriver.go into our codebase (and can confirm that it works as expected) so there's no hurry precisely but I'd love to know if you consider this acceptable.

@tam7t
Copy link
Contributor

tam7t commented Dec 22, 2022

@n-oden thanks for the contribution! this seems reasonable I just want to test it out manually some and see the behavior if the metrics are already defined in stackdriver as a gauge. Depending on that behavior I may want to provide some docs on deleting metrics or some migration path.

@n-oden
Copy link
Contributor Author

n-oden commented Dec 22, 2022

@tam7t thanks! I'll revert out the go.mod changes that seem to have broken some of the tests. (That was a result of my reflexively running go mod tidy in my local workspace -- I don't think the PR depends on any of it.)

This addresses #18

While using `|align delta_gauge()` in MQL is potentially a workaround,
it suffers from the drawback of producing huge negative deltas when
the counter resets, and the current behavior is unhappily surprising to
first-time users who would reasonably assume that `IncrCounter()` would
produce a CUMULATIVE metric.

Signed-off-by: Nathan J. Mehl <n@oden.io>
@n-oden
Copy link
Contributor Author

n-oden commented Dec 22, 2022

@tam7t oh, and btw I can tell you what the behavior will be with existing gauge metrics: an error from the GCM driver that says it can't update the metric due to a metric_kind mismatch. :(

Happy to write up a note explaining things -- should it go in the README or elsewhere?

@n-oden
Copy link
Contributor Author

n-oden commented Jan 2, 2023

@tam7t happy new year! I've updated the README to give people a heads up about the changed behavior.

@tam7t tam7t merged commit 6b3e2bb into google:main Jan 6, 2023
@tam7t
Copy link
Contributor

tam7t commented Jan 6, 2023

Thank you for adding the instructions! Merging now, it may take me a bit to cut a new release but it should be fetchable from HEAD now!

@n-oden
Copy link
Contributor Author

n-oden commented Sep 20, 2023

Hey @tam7t following up here -- any chance that we could get an 0.6.0 release cut? In addition to wanting the fixes in this PR, the go-metrics package itself has moved from github.com/armon/ to github.com/hashicorp/, so v0.5.0 now presents some dependency issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants