Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

re-uses w3c traceparent parser from brave #532

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

codefromthecrypt
Copy link
Contributor

This also highlights a very significant behavior bug which implies folks aren't using this with messaging, or using B3 instead.

Adrian Cole added 2 commits January 12, 2024 09:08
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt
Copy link
Contributor Author

note this also uses import brave.internal.baggage.BaggageFields; without shading, which puts it at drift risk. Possibly better to dispatch to a field of type brave.baggage.BaggagePropagation or possibly after more thought, move to the correct tracestate parser, which hides the "b3" key, but propagates it, so that the extra flags unrepresentable by w3c (intentionally as I asked for years on this), can be kept for outbound.

@marcingrzejszczak
Copy link
Contributor

This also highlights a very significant behavior bug which implies folks aren't using this with messaging, or using B3 instead.

AFAIR W3C + Brave is the default propagation format in Boot and people are using this with messaging successfully 🤷 , regardless I'm all for not having to support additional code and if all the tests are passing I'm all for merging this

@marcingrzejszczak marcingrzejszczak added this to the 1.3.0-M1 milestone Jan 12, 2024
@marcingrzejszczak marcingrzejszczak merged commit e44e4bc into micrometer-metrics:main Jan 12, 2024
5 checks passed
@codefromthecrypt
Copy link
Contributor Author

thanks for merging. I made a note again about the uber bug here, you can decide when to address it as it is 100pct an uber bug and in no way subjective #532 (comment)

@codefromthecrypt
Copy link
Contributor Author

ps the reason this never happens in brave hosted instrumentation is because there is a kind-specific means to inject and extract stuff, as well how to create the consumer span. The upstream code will ignore the shared flag (e.g. never use joinSpan even if it is shared).

The problem is that 3rd party instrumentation may not be careful like this. Particularly, sometimes things I don't remember spring integration or something can be confused about what kind the span is as the abstraction can wash away the details.

The best way to prevent issues is to be context sensitive and put in tests that consumer side (span kind == consumer) never attempt to join a span. They might be here, but yeah setting shared unconditionally as this issue shows is a smell.

EOF and last I'll talk about this promise! I hope it helps future somebody.

@jonatan-ivanov jonatan-ivanov added the enhancement New feature or request label Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants