Adding GC stats to runmetrics plugin #1207
Conversation
|
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 What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| return nil, err | ||
| } | ||
|
|
||
| memStats.pauseTotalNs, err = producer.createInt64GaugeEntry("process/pause_total", "Cumulative milliseconds spent in GC stop-the-world pauses", metricdata.UnitMilliseconds) |
There was a problem hiding this comment.
duplicate. Same as line 263.
There was a problem hiding this comment.
All cumulative values can use AddInt64Cumulative
this allows to compute rate on certain backends.
There was a problem hiding this comment.
The problem with AddInt64Cumulative is that it expects an incremental value, which is not how this metric can be recorded. These would be similar to how memMalloc and memFrees work in this producer in that the runtime is incrementing for you and all that you can record is the total value.
There was a problem hiding this comment.
In that case you could use AddInt64DerivedCumulative.
There was a problem hiding this comment.
I finally did this, I also did this retroactively for the metrics that I believe are also cumulative in this runmetrics package since it seems weird to do this only for net new metrics.
There was a problem hiding this comment.
Sorry for the long delay. The problem with retroactively changing the type for existing metrics is that it will fail for some backends. For example, stackdriver will reject the same metric if the type is different. But on the other hand it is the right thing to do. We'll have add release note as breaking change for this plugin.
Alternatively, it could support both options. Provide a flag to choose the option.
There was a problem hiding this comment.
Makes sense, I'll add a flag for this to ensure people can migrate at their own pace. I'm a stickler for consistency so I'd rather have that than some metrics following a newer standard while others didn't.
There was a problem hiding this comment.
However, should this flag control only be for the ones necessary to stay passive or all cumulative metrics in this package? I lean towards the latter but I can understand why someone would want the former.
There was a problem hiding this comment.
Not really happy with this but honestly rather than have a bunch of if statements running around it made sense to have separate structures.
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@TrevinTeacutter would you mind updating this branch one more time, so that Github Actions can run CI? Thanks again for your patience! |
|
@punya np and done! |
Solves (hopefully): #1188