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

Introduces SpanIdGenerator, used to control id generation. #110

Closed
wants to merge 1 commit into from

Conversation

codefromthecrypt
Copy link
Member

There are a few use cases that support decoupling ID generation

  • allow an alternate random generator, or change its seed to avoid collisions
  • opt out of span id == trace id for the root span
  • make trace-id a request-id, for log correlation, regardless of sampling
  • make Brave easier to test

See openzipkin/zipkin#849

@codefromthecrypt
Copy link
Member Author

cc @eirslett @yurishkuro

@yurishkuro
Copy link

Hm, the difficult problem here is the encoding of the IDs on the wire once they are made opaque. Right now Brave doesn't have that since SpanId class still prescribes a Zipkin-esque structure. If you make SpanId completely opaque like I have in opentracing-go, you have to address two issues, (a) provide pluggable serialization mechanism (e.g. StringPickler in opentracing-go), and (b) find a way to deal with binary protocols, such as TChannel, which assumes tracing info to be of fixed length, and specifically to be Zipkin-esque. That last part is an unfortunate design choice in TChannel, imo, but the fixed length within a binary frame can be a common limitation.

Just curious, are you suggesting one of the ID implementations to be path-like IDs a.b.c.d? What is the benefit of it over Zipkin id + parentID scheme?

* can encode where in the call tree the span originates from, or use alternate
* random number generators.
*/
// @FunctionalInterface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncomment?

(edit): oh, brave supports 1.7.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL updated the comment!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess adding a note like // @FunctionalInterface (commented out because of Java 7 support) or // @FunctionalInterface (uncomment after dropping Java 7 support) would help. But it's a minor thing :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this last comment before seeing your changes. It's ok :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Nov 19, 2015 via email

@codefromthecrypt
Copy link
Member Author

Ex at Twitter, different random seeds were used by device to avoid
collisions. This was done in zipkin format by controlling the random
generator.

Also trace ids are not mutated once created, so when brave creates a trace,
no chance something will change its encoding (inside the long) as the
encoding of long is currently fixed in zipkin v1 as big endian via thrift
tbinary protocol. This particular feature is not a high level goal of mine,
just something I heard from Eirik.

@eirslett
Copy link
Contributor

Changing the way we generate span IDs was a suggestion from the Budapest workshop, Autolytics' software memory did something similar.
The idea is to encode the position in an execution tree, like "0.0.1.4.7", in the span ID. That value would be encoded as a long, so the span ID would always be a long (and not an arbitrary structure).

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Nov 19, 2015 via email

@codefromthecrypt
Copy link
Member Author

I'll remove the "encoding" part as I think this doesn't solve that problem as I now understand it. cheers.

There are a few use cases that support decoupling ID generation

* allow an alternate random generator, or change its seed to avoid collisions
* opt out of span id == trace id for the root span
* make trace-id a request-id, for log correlation, regardless of sampling
* make Brave easier to test
@codefromthecrypt
Copy link
Member Author

ok code is the same, but I updated the description/commit message

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Nov 20, 2015 via email

@codefromthecrypt
Copy link
Member Author

Here's some rationale from @jcarres-mdsol about trace-id vs span-id

in Ruby we have something like "if I do not know the route you are talking about do nothing" . To avoid sending traces when scripts kiddies scan our apps for security holes. But we will even change that to "first create new span and properly set ids, then open thread, now we deal with everything zipkin related".
i.e creating new spans and ids is an operation that should always be successful and happen before we call anything else in this request.
But zipkin traces may depend on if the request is valid or on sample rate and may happen in a different thread, no problem

@codefromthecrypt
Copy link
Member Author

closing for lack of interest and need. can always re-open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants