Support adding process tags in OTEL via env variable #2220
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #2220 +/- ##
=======================================
Coverage 96.18% 96.18%
=======================================
Files 219 219
Lines 10632 10632
=======================================
Hits 10226 10226
Misses 351 351
Partials 55 55
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| func getTags(v *viper.Viper) map[string]string { | ||
| tagsLegacy := flags.ParseJaegerTags(v.GetString(resourceTagsLegacy)) |
There was a problem hiding this comment.
Here is the handling for the legacy tag jaeger.tags. I am fine keeping this for a release or two. It's shot piece of code.
It is possible, however it might affect user configuration in OTEL config. Let me explain it. When |
pavolloffay
force-pushed
the
otel-process-tags
branch
from
0d5aeda to
1d5d711
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay
force-pushed
the
otel-process-tags
branch
from
1d5d711 to
8927e8a
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
cmd/opentelemetry-collector/app/processor/resourceprocessor/resource_processor.go
Outdated
Show resolved
Hide resolved
| // Agent tags | ||
| agentTagsDeprecated = "jaeger.tags" | ||
| // AgentTagsDeprecated is a configuration property name for adding process tags to incoming spans. | ||
| AgentTagsDeprecated = "jaeger.tags" |
There was a problem hiding this comment.
Should this be named JaegerTagsDeprecated as not specific to agent?
Assuming not specific to agent - the changes seem limited to the agent config at the moment - shouldn't they also be applied to collector?
There was a problem hiding this comment.
Collector uses only --collector.tags
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
| config map[string]interface{} | ||
| service configmodels.Service | ||
| }{ | ||
| { |
There was a problem hiding this comment.
Sorry should have mentioned in preview review - there needs to be a test without the flag being specified to confirm resource processor is not added.
There was a problem hiding this comment.
that was my intention when I refactored the test to use arrays, but I apparently forgot to add the second use case.
| @@ -56,17 +56,20 @@ func CollectorConfig(storageType string, zipkinHostPort string, factories config | |||
| recTypes = append(recTypes, string(v.Type())) | |||
| } | |||
| hc := factories.Extensions["health_check"].CreateDefaultConfig() | |||
| resProcessor := factories.Processors["resource"].CreateDefaultConfig() | |||
There was a problem hiding this comment.
Isn't the same approach going to be used for collector, as with the agent default config, to only use the resource processor if labels defined?
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Resolves #2161
This PR makes configurable collector/agent tags via env variables(or any viper manged config).
Changes:
resource.tags. This name aligns with OTEL naming conventions.Why haven't I used the current flags
jaeger.tags/collector.tags/agent.tags?This PR #1854 (comment) added
collector.tags,agent.tagsand deprecatedjaeger.tags. It does not seem right to offer the same functionality via 2 different configuration properties. It creates confusion and additional (dedup) handling in all-in-one.