Add ability to optionally translate metrics in signalfx exporter #477
Conversation
dmitryax
force-pushed
the
enable-sfx-translations
branch
from
1f93547
to
a975208
Compare
Codecov Report
@@ Coverage Diff @@
## master #477 +/- ##
===========================================
+ Coverage 71.09% 85.48% +14.39%
===========================================
Files 14 181 +167
Lines 602 9695 +9093
===========================================
+ Hits 428 8288 +7860
- Misses 150 1088 +938
- Partials 24 319 +295
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
dmitryax
force-pushed
the
enable-sfx-translations
branch
2 times, most recently
from
f300312
to
2d68b43
Compare
| ) (sfxDataPoints []*sfxpb.DataPoint, numDroppedTimeSeries int) { | ||
| sfxDataPoints, numDroppedTimeSeries = metricDataToSfxDataPoints(logger, md) | ||
| if metricTranslator != nil { | ||
| metricTranslator.TranslateDataPoints(sfxDataPoints) |
There was a problem hiding this comment.
do you think this translation and the sanitization could be moved to metricDataToSfxDataPoints? Seems like since we already iterate over the datapoints in that method, we can avoid going over it again? What do you think?
There was a problem hiding this comment.
I don't see any benefit from moving it to metricDataToSfxDataPoints to be honest. Additional iteration doesn't seem to expensive comparing to all the other logic, and I'd really like to separate out all the additional metric translations from the core data conversion logic. At some point, once backend supports all the Otel convention we should be able to remove these couple of lines, instead of going through the data conversion logic.
There was a problem hiding this comment.
I agree some of the translations we may want to do would fit better as a post processing step. But in this case I guess if we do these translations before converting consumerdata.MetricsData to []*sfxpb.DataPoint, doing the mapping once per consumerdata.MetricsData is sufficient whereas later on we have to do it once per dimension per sfxpb.DataPoint. Thoughts?
There was a problem hiding this comment.
We cannot mutate consumerdata.MetricsData because it'll affect other exporters that user sets in the pipeline, so in that case would need to copy consumerdata.MetricsData objects before making any modifications. That's why I think it's better to do translation on final data structure that is already a copy and can be mutated.
There was a problem hiding this comment.
Also translation that can be done once on consumerdata.MetricsData is resource/node labels renaming only, which is pretty small part comparing to all the other translations we have to do: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/476/files#diff-a964270a3bd3600cfd48aa1d7f5f856f
There was a problem hiding this comment.
I wasn't recommending we update consumerdata.MetricsData but rather do the translation alongside the consumerdata.MetricsData -> []*sfxpb.Datapoint conversion. However, if you don't think we'll gain a lot in terms efficiency, I'm good with this approach.
|
|
||
| // TranslationRules defines a set of rules how to translate metrics to a SignalFx compatible format | ||
| // If not provided explicitly, the rules defined in translations/config/default.yaml are used. | ||
| TranslationRules []translation.Rule `mapstructure:"translation_rules"` |
There was a problem hiding this comment.
Could you also update the README to include these options?
dmitryax
force-pushed
the
enable-sfx-translations
branch
2 times, most recently
from
5dffca7
to
a4ce62d
Compare
dmitryax
force-pushed
the
enable-sfx-translations
branch
2 times, most recently
from
bfebcf0
to
346b008
Compare
This commit provides ability to translate metrics in siganlfx exporter. If an optional "send_compatible_metrics" configuration flag is enabled signalfx exporter translates metrics before sending them out according to `translation_rules` configuration. If "translation_rules" field is not set, default translation rules are used that defined in exporter/signalfxexporter/translation/constants.go. Only dimension renaming is added in this commit to validate the approach. Dimension translation applied to metric dimensions as well as to metadata properties update. Other translation rules will be added in the following commits
dmitryax
force-pushed
the
enable-sfx-translations
branch
from
346b008
to
b0ad531
Compare
This change fixes inconsistencies in component interfaces. Motivation: - Uniformness results in reduction of code that currently has to deal with differences. - Processor.Start is missing and is important for allowing processors to communicate with the Host. What's changed: - Introduced Component interface. - Unified Host interface. - Added a Start function to processors (via Component interface). - Start/Shutdown is now called for Processors from service start/shutdown. - Receivers, Exporters, Processors, Extensions now embed Component interface. - Replaced StartTraceReception/StartMetricsReception by single Start function for receivers. - Replaced StopTraceReception/StopMetricsReception by single Shutdown function for receivers. Note: before merging this we need to announce the change in Gitter since it breaks existing implementations in contrib (although the fix is easy). Resolves #477 Resolves #262
Description:
This PR provides ability to translate metrics in siganlfx exporter. If an optional "send_compatible_metrics" configuration flag is enabled, signalfx exporter translates metrics before sending them out according to
translation_rulesconfiguration. Iftranslation_rulesfield is not set, default translation rules are used that defined in exporter/signalfxexporter/translation/constants.go.Only dimension renaming is added in this commit to validate the approach. Dimension translation applied to metric dimensions as well as to metadata properties update. Other translation rules will be added in the following PRs.