Support adding process tags in OTEL via env variable#2220
Support adding process tags in OTEL via env variable#2220pavolloffay merged 5 commits intojaegertracing:masterfrom
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.
objectiser
left a comment
objectiser
left a comment
There was a problem hiding this comment.
LGTM - only comment is that the resource processor is being added regardless of whether there are any tags to be added. Would it be possible to only add if the tags flag was specified?
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.
| 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>
2 participants
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.