Switch counter metrics to use CUMULATIVE

[prog8]

fixes #18

[google-cla]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[tam7t]

I'll need to do some manual testing on this because i'm not sure that converting a metric from gauge to cumulative is backwards compatible for existing metrics

[tam7t]

I think for CUMULATIVE the points also need to have a StartTime:

https://cloud.google.com/monitoring/api/ref_v3/rest/v3/projects.metricDescriptors#metrickind

[prog8]

@tam7t let me know what are your findings when you do simple backwards compatibility testing

[rf]

So I tested this and I'm getting this error
Failed to write time series data: rpc error: code = InvalidArgument desc = Field timeSeries[0].points[0].interval.start_time had an invalid value of "2021-08-31T14:19:47-07:00": The start time must be before the end time (2021-08-31T14:19:47-07:00) for the non-gauge metric 'custom.googleapis.com/go-metrics/my-metric-id
and yes, this is a backwards incompatible change -- I had to manually delete the old metric since the type changed.

[rf]

The correct MetricKind may actually be DELTA.

[prog8]

@rf thanks for your effort.
@tam7t would you accept PR afte changing MetricKind to DELTA?

EDIT: I think it's better to avoid forking this repo to fix it. Ideally if it stays under Google's control but I see there is not much info from Google side 😞

[tam7t]

#19 (comment) is the reason for the error there. I'm still evaluating the effects of changing the metric types at all but i do not suspect that DELTA will make it more difficult as additional time periods would need to be accounted for

[rf]

It turns out DELTA metrics are not supported in Stackdriver for custom metrics.

I'm not sure that counters in go-metrics are intended to be treated like a statsd counter (i.e. something that resets after an interval). It's not clear if other sinks treat it this way.

// edit: Was looking at the wrong go-metrics library (lol). The statsd sink does increment a statsd counter when you call IncrCounter. So this library has counters implemented incorrectly. When we do a deep copy of the stats in order to report, it should be clearing the counters. I think once that is implemented, using a GAUGE to report them might actually work fine.

[tam7t]

@rf I believe that what you are describing is how to implement and report a DELTA metric (clearing each interval and reporting the start/end time of the interval) because you are reporting the change of the counter over a specific time interval. To report a counter as CUMULATIVE you need to report the start time of the accumulation in the report to stackdriver, as the histogram metrics do.

[rf]

I did have some trouble getting CUMULATIVE to work -- I was also trying to use a StartTime that was basically the end of the last reporting interval. Not sure if that was right, should I be using the same start time as for histograms?

I was able to get this to work, however, by using GAUGE and just resetting the counters every time we do the deep copy in order to flush. It has the benefit of being somewhat backwards incompatible (because the metrics don't need to be deleted from stackdriver) but will end up reporting very different values.