Fix TODOs in Jaeger exporter spec #1374
Conversation
yurishkuro
changed the title
|
cc @jkwatson |
|
|
||
| In Jaeger Thrift format the timestamps and durations MUST be represented in | ||
| microseconds (since epoch for timestamps). If the original value in OpenTelemetry | ||
| is expressed in nanoseconds, it MUST be rounded to microseconds. |
There was a problem hiding this comment.
Is floor acceptable instead of rounding mathematically? e.g., what happens normally when doing TimeUnit.NANOSECONDS.toMicros in Java
| * use Span start time as the timestamp of the Log | ||
| * set Log tag `event=link` | ||
| * set Log tags `trace_id` and `span_id` from `SpanContext`'s fields | ||
| * store `Link`'s attributes and Log tags |
There was a problem hiding this comment.
| * store `Link`'s attributes and Log tags | |
| * store `Link`'s attributes as Log tags |
|
|
||
| OpenTelemetry `Link`(s) MUST be converted to `SpanReference`(s) in Jaeger, | ||
| using `FOLLOWS_FROM` reference type. The Link's attributes cannot be represented | ||
| in Jaeger explicitly. The exporter MAY convert link attributes to span `Log`(s): |
There was a problem hiding this comment.
| in Jaeger explicitly. The exporter MAY convert link attributes to span `Log`(s): | |
| in Jaeger explicitly. The exporter MAY convert links to span `Log`(s) to preserve attributes: |
Looking at the below conversion, it seems like it is more than just attributes being stored in Log (trace_id, span_id)
| | `SpanKind.SERVER`|`"server"`| | ||
| | `SpanKind.CONSUMER`|`"consumer"`| | ||
| | `SpanKind.PRODUCER`|`"producer"` | | ||
| | `SpanKind.INTERNAL`| do not add `span.kind` tag | |
There was a problem hiding this comment.
Collector currently adds "span.kind=internal" in this case [1]. Can you please clarify why it should not be doing this? I assume because "internal" is not a valid value that Jaeger can expect to see?
There was a problem hiding this comment.
It's fine either way, there's no special processing for "internal". We can allow both, or for consistency require "internal".
There was a problem hiding this comment.
My preference is to not include internal since it increases data size without adding any useful info.
There was a problem hiding this comment.
If you do not mind I would prefer not to change Collector's translation since I am worried that it may break people's stuff who rely on the current behavior (unlikely but possible). So, if using "internal" does not result in any problems for Jaeger let's allow it.
| OpenTelemetry `Link`(s) MUST be converted to `SpanReference`(s) in Jaeger, | ||
| using `FOLLOWS_FROM` reference type. The Link's attributes cannot be represented |
There was a problem hiding this comment.
Collector implementation specifically calls out that the parent span reference should be represented as CHILD_OF and must be at the first place because it is important for Jaeger backend [1]. Is this correct? If so let's also emphasize here.
There was a problem hiding this comment.
What you're describing is the behavior needed to represent ParentID, and I will clarify that it must be the first in the list. But this is a section for additional Links.
yurishkuro
force-pushed
the
jaeger-1237
branch
from
3ed71bf
to
720082f
Compare
|
btw, @tigrannajaryan , I wouldn't mind pointing to Collector's code as the reference implementation of this conversion, any concerns? |
@yurishkuro No concerns, I think it is a good idea. Here is the link: https://github.com/open-telemetry/opentelemetry-collector/blob/master/translator/trace/jaeger/traces_to_jaegerproto.go |
* Expand Jaeger exported spec * delint * big endian * no hard tabs * review comments * order of references * reference collector
Provide additional details for mappings needed to be supported in Jaeger exporters.
Related issues #1237