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

provides support for TracingMetadata #824

Merged
merged 2 commits into from
May 11, 2020

Conversation

OlegDokuka
Copy link
Member

closes #583

Signed-off-by: Oleh Dokuka shadowgun@i.ua

@OlegDokuka OlegDokuka added this to the 1.0 milestone May 7, 2020
@OlegDokuka
Copy link
Member Author

OlegDokuka commented May 7, 2020

cc\ @adriancole your feedback is really appreciated (also see https://github.com/rsocket/rsocket/blob/master/Extensions/Tracing-Zipkin.md for more info)

@codefromthecrypt
Copy link

codefromthecrypt commented May 8, 2020

I'd suggest focusing on the outputs vs the inputs. Ex instead of describing features like isSamplingEnabled and isDebuggingEnabled (knowing there may be different sampling algos etc).. focus only on the resulting data that you'd take action on.

isReported -> isSampled: https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/SamplingFlags.java#L83

if you want to support debug you should also know the difference between this and sampled
isDebug: https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/SamplingFlags.java#L108

Not now, but just fyi, there's a third option which is that data is sampled, but it is not propagated in primary trace headers/frames. This is for recording everything for local processing such as metrics or secondary sampling

isSampledLocal:
https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/SamplingFlags.java#L100

Hope this helps!

@spencergibb
Copy link
Contributor

He's trying to implement the extension spec he mentioned.

@codefromthecrypt
Copy link

codefromthecrypt commented May 8, 2020

sorry missed that link.. I saw "Add Zipkin Tracing Flyweight" and had no idea what it meant.

In that case, just retain comments about isSampled and isDebug and forget about isSampledLocal as that's not propagated.

@OlegDokuka
Copy link
Member Author

Alright, by looking more into all the stuff I tend to say that current RSocket spec is incorrect and misaligned with today's implementation of TraceContext that I found at brave. Fixing that first and then continue on this impl

@OlegDokuka OlegDokuka marked this pull request as draft May 8, 2020 06:55
@OlegDokuka
Copy link
Member Author

@adriancole do you have a few minutes to chat on the TraceContext topic and discuss all the flags that it has and their importance / behavior?

@codefromthecrypt
Copy link

ok find me here https://gitter.im/openzipkin/zipkin

@OlegDokuka OlegDokuka force-pushed the enhancement/tracing-metadata branch from a5ae918 to d68b542 Compare May 11, 2020 16:58
@OlegDokuka OlegDokuka marked this pull request as ready for review May 11, 2020 16:59
@OlegDokuka OlegDokuka force-pushed the enhancement/tracing-metadata branch 3 times, most recently from 4c97694 to 419c5d1 Compare May 11, 2020 17:23
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
Signed-off-by: Oleh Dokuka <shadowgun@i.ua>
@OlegDokuka OlegDokuka force-pushed the enhancement/tracing-metadata branch from 7b2438b to 778d46a Compare May 11, 2020 22:33
@OlegDokuka OlegDokuka merged commit 08c2a78 into develop May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Zipkin Tracing Flyweight
4 participants