Translate OC resource labels to Jaeger process tags #325
Conversation
owais
requested review from
bogdandrutu,
flands,
pjanotti,
songy23 and
tigrannajaryan
as code owners
Codecov Report
@@ Coverage Diff @@
## master #325 +/- ##
=========================================
+ Coverage 74.2% 74.31% +0.1%
=========================================
Files 115 115
Lines 6676 6704 +28
=========================================
+ Hits 4954 4982 +28
Misses 1474 1474
Partials 248 248
Continue to review full report at Codecov.
|
songy23
left a comment
songy23
left a comment
There was a problem hiding this comment.
Also, in practice, it's going to be very rare for people to translate from
OC to Jaeger and then back from Jaeger to OC in the same data
pipeline.
Agree, I would not worry too much about the Jaeger-to-OC case.
songy23
left a comment
songy23
left a comment
There was a problem hiding this comment.
Some minor comments, otherwise LGTM.
|
Thanks for the review @songy23. Updated the PR. |
|
I still don't have merge rights. Can someone please merge this? :) |
| }) | ||
| } | ||
| for k, v := range resource.GetLabels() { | ||
| if v != "" { |
There was a problem hiding this comment.
Why are we ignoring empty values?
There was a problem hiding this comment.
I don't have a strong opinion on this. I suggest ignoring them because for OTel resources not all labels are required (e.g pod_id is optional in gke_container resource). For labels without a value I don't think they're pretty useful.
There was a problem hiding this comment.
I don't have a strong opinion either but I think it's probably better to allow it. I don't see a reason why anyone would use an empty string as a value but I don't see much reason in disallowing it either. If something does not have any obvious downsides, I think we should generally try to not limit flexibility for end users. The check for resource type is a different thing as empty string there means it was never set.
| }) | ||
| } | ||
| for k, v := range resource.GetLabels() { | ||
| if v != "" { |
Preserve OC resource labels during translation to Jaeger by adding them
to the process tags
Right now we translate OC node to Jaeger process while completely
ignoring OC resource labels resulting in loss of this data. This PR
patches the OC > Jaeger translators by making sure the resource labels
are recorded as Jaegerprocess tags. While this preserves process labels
and prevents loss of such data when translating to Jaeger, it's not a
perfect solution because:
1. It will replace any tags from OC Node with the same names.
Current behaviour is to give precedence to resource labels over node
attributes. We can make it configurable or revisit the decision
later.
2. Translation is not reversible
Translating back from Jaeger to OC will still not re-create the OC
process object. It'll instead translate all the Jaeger process tags
to OC node attributes. We could add some semantic convention to
denote resource labels differently or add an additional tag to
specify the resource tag names but doing so would add additional
complexity to backends processing traces in the Jaeger format. Also,
in practice, it's going to be very rare for people to translate from
OC to Jaeger and then back from Jaeger to OC in the same data
pipeline.
While this doesn't solve the cases perfectly, we think it's better than
completely dropping the resource data.
|
@tigrannajaryan @songy23 can we merge this please? It's kind of blocking me from working on another feature :) |
|
@owais Nice commit message! |
…tribute vs setAttributes (open-telemetry#325)
…metry#325) * Expose initial and effective config for debugging purposes (Still incomplete, just the initial ideas) * Add tests * Remove outdated test comment * Add 'token' to the redaction list * Require an env var to be defined before serving config
4 participants
Preserve OC resource labels during translation to Jaeger by adding them
to the process tags
Right now we translate OC node to Jaeger process while completely
ignoring OC resource labels resulting in loss of this data. This PR
patches the OC > Jaeger translators by making sure the resource labels
are recorded as Jaegerprocess tags. While this preserves process labels
and prevents loss of such data when translating to Jaeger, it's not a
perfect solution because:
It will replace any tags from OC Node with the same names.
Current behaviour is to give precedence to resource labels over node
attributes. We can make it configurable or revisit the decision
later.
Translation is not reversible
Translating back from Jaeger to OC will still not re-create the OC
process object. It'll instead translate all the Jaeger process tags
to OC node attributes. We could add some semantic convention to
denote resource labels differently or add an additional tag to
specify the resource tag names but doing so would add additional
complexity to backends processing traces in the Jaeger format. Also,
in practice, it's going to be very rare for people to translate from
OC to Jaeger and then back from Jaeger to OC in the same data
pipeline.
While this doesn't solve the cases perfectly, we think it's better than
completely dropping the resource data.