-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[exporter/signalfx] Remove deprecated translation_rules configuration option #35332
base: main
Are you sure you want to change the base?
[exporter/signalfx] Remove deprecated translation_rules configuration option #35332
Conversation
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: | | ||
Please use the processors to handle desired metric transformations instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to recommend a specific processor here, but it looks like some operations can be done with the resource, metricstransform, or transform processor, so I didn't know if it would be worth listing all three.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to have guidelines on how to replace every translation rule with examples
const ( | ||
translationRulesConfigKey = "translation_rules" | ||
) | ||
type translationRulesConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mimics the original translation_rules
config option, but is not reachable by user configuration. It's only used by defaultTranslationRules
to parse the YAML into a []translation.Rule
object.
|
||
var defaultTranslationRules = func() []translation.Rule { | ||
cfg, err := loadConfig([]byte(translation.DefaultTranslationRulesYaml)) | ||
var data map[string]any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New functionality here is essentially copied from loadConfig
. loadConfig
takes YAML and parses it into the main Config
object, but this function only cares about the now deleted translation_rules
option, so it can't parse the YAML into a Config
object anymore.
Putting this on hold to properly replace the |
Picking this back up as the |
8beba1c
to
f81ef77
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35332 +/- ##
==========================================
- Coverage 79.61% 79.60% -0.01%
==========================================
Files 2252 2252
Lines 211894 211892 -2
==========================================
- Hits 168694 168680 -14
- Misses 37531 37542 +11
- Partials 5669 5670 +1 ☔ View full report in Codecov by Sentry. |
a2cc68e
to
11e5523
Compare
| -----------------|--------------------|----------------------| | ||
| rename_dimension_keys | `metricstransform` processor's update label function | [one metric](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/metricstransformprocessor#rename-labels), [multiple metrics](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/metricstransformprocessor#rename-labels-for-multiple-metrics) | | ||
| rename_metrics | `metricstransform` processor's rename metric functionality | [one metric](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/metricstransformprocessor#rename-metric), [multiple metrics](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/metricstransformprocessor#rename-multiple-metrics-using-substitution) | | ||
| multiply_int, divide_int, multiply_float | `metricstransform` processor's `scale` value functionality | [one metric](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/metricstransformprocessor#scale-value) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The metricsgeneration processor could also be used here. I chose metricstransform
processor instead as we're recommending it for so many other options.
Description:
The
translation_rules
option has been deprecated since #18218, we can now delete the option.Internally, to be able to still have the default translation rules without the
translation_rules
config option, a new config structure has been added, but is not nested within the exporter'sConfig
, it's its own struct.