Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Add new metric types #13

Closed
wants to merge 1 commit into from

Conversation

noly
Copy link

@noly noly commented May 6, 2020

Add new metric calculation for

  • rate
  • cumulative rate
  • cumulative count

add rate and cumulative-rate
@noly noly marked this pull request as draft May 6, 2020 14:39
@noly noly marked this pull request as ready for review May 6, 2020 14:40
// timestamps of multiple calls. If this is the first time the name/attributes
// combination has been seen then the `valid` return value will be false.
func (dc *DeltaCalculator) CountMetric(name string, attributes map[string]interface{}, val float64, now time.Time) (count telemetry.Count, valid bool) {
func (dc *DeltaCalculator) GetCumulativeCount(name string, attributes map[string]interface{}, val float64, now time.Time) (count telemetry.Count, valid bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a backwards incompatible change and it does not appear to be warranted. 👎

@@ -0,0 +1,15 @@
package internal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating an internal package is not adding any benefit. It is encapsulating already un-exported structures and does not add naming/functional benefit.

// If no previous timestamp is NOT found, returns false (as no calculation is made)
// If a previous timestamp is found use it to get the elapsed time (in seconds) and use that as the denominator
// Rate = value / (now - before)[s]
func (rc *RateCalculator) GetRate(name string, attributes map[string]interface{}, val float64, now time.Time) (gauge telemetry.Gauge, valid bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exporting a rate from a remote location only serves to decrease the usability of this data. You cannot reverse this operation without also transmitting the time interval it was calculated with. New Relic has the functionality to take one of the existing Metrics and calculate rates (as well as other computed values) from these.

Additionally, this would add an inconsistency in this telemetry-sdks API from the rest of the New Relic telemetry-sdks. If we want to add this metric type it need to be addressed in the specification

@noly
Copy link
Author

noly commented Sep 14, 2020

We have moved the calculations into the agent

@noly noly closed this Sep 14, 2020
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.

None yet

3 participants