open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
2.92k stars 2.28k forks source link

[exporter/signalfx] Remove direction references from metric translations #12641

Closed crobert-1 closed 1 year ago

crobert-1 commented 2 years ago

This is a subtask of issue https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/11815

The signalfxexporter has functionality that "translates" input metrics into output metrics, and the default translation functionality references the direction attributes. These references should be updated for the new metric format.

crobert-1 commented 2 years ago

One thing that should be noted: Metrics can't be identically matched to previous form.

Originally, one metric (system.disk.io for example) might have many datapoints associated with it, both of read and write directions. After doing a sum aggregation without the direction and device attribute/dimension, all datapoints would be merged into a single datapoint, with the metric name system.disk.io.total.

This is no longer possible as read and write metrics will be in individual metrics. This means that there may be two metrics labeled system.disk.io.total, where one has a single datapoint that is the sum of the read and the other has a single datapoint that is the sum of the write. Neither would have any label/dimension to show which is which, they'd look identical.

If this is not an acceptable solution I'll have to rework the change presented in #12642.

github-actions[bot] commented 2 years ago

Pinging code owners: @pmcollins @dmitryax

crobert-1 commented 2 years ago

One thing that should be noted: Metrics can't be identically matched to previous form.

Originally, one metric (system.disk.io for example) might have many datapoints associated with it, both of read and write directions. After doing a sum aggregation without the direction and device attribute/dimension, all datapoints would be merged into a single datapoint, with the metric name system.disk.io.total.

This is no longer possible as read and write metrics will be in individual metrics. This means that there may be two metrics labeled system.disk.io.total, where one has a single datapoint that is the sum of the read and the other has a single datapoint that is the sum of the write. Neither would have any label/dimension to show which is which, they'd look identical.

If this is not an acceptable solution I'll have to rework the change presented in #12642.

Dmitrii and I discussed this problem and decided the best way to solve this is to modify the translation logic to operate on multiple metrics at once.

Currently the translateAndFilter method within the exporter/signalfxexporter/internal/translation/converter.go is called for each metric, so operations can only be done on datapoints within the same metric. Changing the logic to allow operations on metrics within the same ScopeMetric will allow aggregations and operations to be done across metrics, allowing metrics to be combined in a way they were before the direction attribute change. This would make it possible for system.disk.io.total to contain both direction dimensions within the same metric as before.

Once this logic has been updated, the translation rules can be updated to properly map new metrics to the previous format.

This change should make it so that the signalfx exporter also excludes the new format of metrics until the signalfx backend properly supports them.

github-actions[bot] commented 1 year ago

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

dmitryax commented 1 year ago

Not needed anymore