cumulativetodeltaprocessor: Reopening #4444 Update cumulative to delta #5772
Conversation
The configuration has been updated so that: * An empty list of metrics defaults to converting all cumulative metrics to delta. * The state TTL (MaxStale) is now considered configurable * There is a configuration option to enable/disable conversion of non-monotonic cumulative metrics. Non-monotonic cumulative metrics are normally considered gauges and aggregation temporality should not be converted. The default is that only monotonic values are converted.
* Uses the internal tracker library instead of AWS metrics library. This enables separation of timeseries by resource and instrumentation library. * Metric data points which are invalid (the first in a series of non-monontic cumulative values) are now removed from the dataset.
By default, the cumulative to delta processor now converts all metrics. Previously, the processor did not convert any metrics unless listed in the configuration.
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
| @@ -24,15 +24,16 @@ import ( | |||
| type Config struct { | |||
| config.ProcessorSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct | |||
|
|
|||
| // List of cumulative sum metrics to convert to delta | |||
| // List of cumulative metrics to convert to delta. Default: converts all cumulative metrics to delta. | |||
There was a problem hiding this comment.
What does it mean "default" here? You mean nil? That will definitely be not ideal.
|
|
||
| The following settings can be optionally configured: | ||
|
|
||
| - `metrics`: The processor uses metric names to identify a set of cumulative sum metrics and converts them to cumulative delta. Defaults to converting all metric names. |
There was a problem hiding this comment.
I do understand the NR need, but this "default" should be what the user expects the most, and I think this is not expected (in my opinion).
There was a problem hiding this comment.
I would strongly suggest split into "changes to default config" vs "new functionality added". I feel the first is a breaking change while the later is not.
Yes, I see your point. I have reverted to the previous behavior. Now only metrics that are explicitly listed in the config are converted from cumulative -> delta.
I will add this as another issue to follow up on. I like @dmitryax's suggestion of changing this to support a regex #4444 (comment). I believe this would serve the need to be able to support converting all metrics by specifying * or something.
| if len(config.Metrics) == 0 { | ||
| return fmt.Errorf("metric names are missing") | ||
| } |
There was a problem hiding this comment.
Should we keep this for the moment?
There was a problem hiding this comment.
Yes, that seems fair. I will re-add.
| @@ -56,7 +56,6 @@ func createMetricsProcessor( | |||
| return nil, fmt.Errorf("configuration parsing error") | |||
| } | |||
|
|
|||
| processorConfig.Validate() | |||
| ilms.RemoveIf(func(ilm pdata.InstrumentationLibraryMetrics) bool { | ||
| ms := ilm.Metrics() | ||
| ms.RemoveIf(func(m pdata.Metric) bool { | ||
| if ctdp.metrics != nil { |
There was a problem hiding this comment.
probably better to never have "nil" but empty map? This is also a logic that we should add later that if the list is empty gets to pass all.
There was a problem hiding this comment.
Since the map cannot be nil or empty anymore, I just removed this check.
Description:
This PR revives #4444 which was closed a few weeks back as Allan Feldman has left the project.
Captured here is a summary of suggestions from the original PR with an indication that I've addressed them or would like to propose them as a followup:
MaxStale->MaxStalenessI have some folks who want to use this component, so I would like to request landing it as is. I will create issues for the suggested followups. Also, I have colleagues who can help pick this work up.