Incorrect conversion to Jaeger model

[yurishkuro]

Bug Report

opentelemetry-dotnet/src/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs

Line 150 in 5fa2f10

var spanServiceName = jaegerSpan.PeerServiceName ?? this.Process.ServiceName;

Symptom

Peer service represents a service YOUR service is talking to. That remote service may emit its own span, but the span generated by YOUR service should have YOUR service name in span.Process.ServiceName.

[CodeBlanch]

This was done intentionally to workaround the gap in Jaeger discussed on jaegertracing/jaeger#2584. Would love to remove this! It would speed up the export greatly to do so. That's why I brought it up on that issue 😄

"Fixing" this though would change the behavior of how spans emitted from OpenTelemetry .NET show up in Jaeger UI. How about we make it configurable? Anyone wanting strict letter-of-the-law API usage can toggle that on if they don't mind their dependency spans showing as internal on the UI?

[ndrwrbgs]

Agreed with original implementation, did the same thing in multiple wrappers to try to work around the UI. If OP is resolute on the existing functionality, putting the modification behind a flag should be a small code change?

[ndrwrbgs]

(side note that this is the primary/only blocker to making this understandable for people outside of day-to-day work in tracing in our company. That the OTel implementation worked around this on our behalf, unblocks us after 5 months as long as we use OTel, as long as this Issue is not corrected)

[yurishkuro]

I don't think faking incorrect data to work around a display issue as an acceptable approach. And the display issue itself is more of a preference for some people, what Jaeger UI does it not wrong (spans emitted from the same service have the same color).

[ndrwrbgs]

I agree, this should be fixed here after Jaeger UI supports the preference flag.

…

On Sun, Nov 8, 2020, 5:13 PM Yuri Shkuro ***@***.***> wrote: I don't think faking incorrect data to work around a display issue as an acceptable approach. And the display issue itself is more of a preference for some people, what Jaeger UI does it not wrong (spans emitted from the same service have the same color). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACSHCORRDWASUC72ZSHO3FLSO4637ANCNFSM4S46AMZA> .

[yurishkuro]

I agree, this should be fixed here after Jaeger UI supports the preference flag.

I disagree. The current behavior of the SDK generates incorrect data, which can never be corrected by the backend.

Allow me to demonstrate why it is already bad even with the thing it is presumably aspires to fix in the UI. Let's say service A generates two spans, a server span on ingress, and a client span when A calls service B. With the current fudging of the service name, the two spans looks like they come from A and B respectively, and displayed in different colors in Jaeger UI. Which may sound nice, except:

• it creates a misleading picture that service B is instrumented. It is not.
• it tells a misleading story about the latency of B. The second span does not represent server-side latency of B, but client side latency from A's point of view.

Now assume that service B adds instrumentation (the current behavior in the SDK has no idea if B is instrumented or not). Now we get TWO spans, A.client and B.server, that look like they came from B, and shown in the same color. Even worse than before. I wouldn't even know how to reason about such trace where an entry span into a service is client kind, this will likely break many other processing tools like dependency graphs, etc.

[ndrwrbgs]

Sounds like Jaeger should update the model so that's not a problem, or be consistent in how it handles when the other service does and doesn't generate the span, eh? If it just provided a way to emphasize/show the remote when it's not instrumented, the services wouldn't have to double-emit and we'd not be in this problem.

[johnduhart]

I have to agree with Yuri here, the exporter should not be emitting spans with an incorrect service name purely for display purposes in a UI. We consume trace data for analysis and monitoring purposes, and accuracy is important.

[ndrwrbgs]

I don't think _anyone_ wants to be emitting incorrect or misleading data; but when a request for usability is declined and sometimes you have to work around the large corporations :)

…

On Mon, Nov 9, 2020, 10:21 PM John Du Hart ***@***.***> wrote: I have to agree with Yuri here, the exporter should not be emitting spans with an incorrect service name purely for display purposes in a UI. We consume trace data for analysis and monitoring purposes, and accuracy is important. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACSHCOXLIJMDRLQEOSWXF4TSPDLXJANCNFSM4S46AMZA> .

[johnduhart]

The place to work around that problem isn't within a single SDK implementation. All of the implementations should work as close to identical as possible, and right now I'm only aware of the .NET SDK having this behavior.

If you want to improve how a trace is presented, fix it at the presentation layer. Please don't munge data to get the result you want.

[yurishkuro]

open-telemetry/opentelemetry-specification#1222