ADR-41 tracing - disabling tracing by rewriting traceparent to Xraceparent

[roeschter]

Setting the traceparent header (usually by a client) enables tracing. In some situation tracing needs to be disabled
For performance reasons the disabling is done by a single character replacement (avoiding a parsen and render header cycle) from traceparent to Xraceparent - This is an undocumented oddity from the end users perspctive.

Situations in which traceparent is disabled

1. When storing in a stream
2. Cross account service export unless tracing is explicitely enabled by the export
3. If only “traceparent” header is found (that is, "Nats-Trace-Dest” is not) and the trace should not be sampled. That is, the account has a sampling value (between 0..100) and the server picks a random number, if below the sampling value the message will be traced, otherwise it will not. In that case, the header is “disabled” (with the “X”).

Case 2) Is not a security feature
Case 3) It is natural to expect that the header is removed is mangled

Problem:
Case 1) Is unexpected and counter-intuitive
a) Unexpected - The consumer is the one interested in the tracing data. But it will not know it was traced as traceparent has been mangled.
b) Why is delivery not traced? One would expect to see the message trace end-2-end. Including source/mirror and consumption.

Suggestion:
Do NOT disable tracing when storing in a stream. Continue tracing all the way through to the consumer.

[ripienaar]

When an account level trace destination is set like here https://github.com/nats-io/nats-architecture-and-design/blob/main/adr/ADR-41.md#trace-context-activation then traceparent is used to trigger a trace, but if we do not rewrite that then every consumer delivery would again be traced.

This may or may not be desirable and no doubt some people now expect the current behaviour so we'd need to make this a opt-out in accounts config and jwts

wdyt @kozlovic

[roeschter]

Suggestion: Make it disabling tracing on write to steam configurable.

Add new field:
jetstream_keep_tracing - Defaults to "false"

accounts {
  A {
    users: [{user: a, password: pwd}]
    msg_trace: {
        dest: "a.trace.subj"
        jetstream_keep_tracing: "true"
        sampling: "100%"
    }
  }
}

[kozlovic]

@ripienaar Yes, every consumer delivery AND if that happens to be routed generate "separate" traces when inbound in the remote servers.

@roeschter I don't think this would even work. There is no code currently to generate a trace to say that it was delivered to a given consumer. But also, a stored message can be "replayed" many times and it would not mean anything to generate traces during those events. They would be kind of "standalone" events without really an origin.

[ripienaar]

However traceparent is also used by other systems to initiate traces to things like Otel, so even if we cant do traces for those users would still like to continue tracing into their forward systems, so if we want to do something here we'd have to somehow prevent those from kicking off our traces but preserve the header?

[kozlovic]

@ripienaar That's what I have been discussing with Michael by email. I could understand not scrapping those headers and instead adding a new header that disables. That is a big change (especially that we will need to rewrite header/msg) but I could understand that. But the disabling (one way or another) needs to be done. So regardless of the rewrite of "traceparent", there is no provision in message tracing to go "down" to the JS consumer. That is not a bug, it was never meant to go there.

[ripienaar]

Yes agree not a bug, the additional header seems the most reasonable?

Also might be generally useful if someone wants to publish a message with a traceparent for Otel purposes but want to prevent nats traces for some random reason.

Does not sound like something we can realistically do mid 2.12

[kozlovic]

Does not sound like something we can realistically do mid 2.12

No. And adding a new header that disables (either internally added instead of 'X', or set by the user) will not be "backward" compatible, meaning in a topology with different server versions, the new header is not going to have any effect. This is of course "transient" in the case of a topology server upgrade, but with leafnodes/gws, it is possible that we have servers at various versions. But anyway, that would be the best we can do to add a header that disables and not mangle the existing ones.

[ripienaar]

I agree though, when evaluating these kinds of things, we assume uniformly shaped clusters that's a requirements we communicate to users/customers so I am ok with the backward compatibility issue here. Despite agreeing it's not a good answer with leaf nodes involved which are often of a different version but that's where we're at.