Adds Cassandra client and server integration #414

codefromthecrypt · 2017-05-12T10:19:16Z

This contains tracing instrumentation for Cassandra and the DataStax Java Driver.

brave.cassandra.Tracing extracts trace state from the custom payload
of incoming requests. How long each request takes, each suboperation,
and relevant tags like the session ID are reported to Zipkin.

brave.cassandra.driver.TracingSession tracks the client-side of cassandra and
adds trace context to the custom payload of outgoing requests. If
server integration is in place, cassandra will contribute data to these
RPC spans.

codefromthecrypt · 2017-05-12T10:20:34Z

cc @openzipkin/cassandra This is intentionally conservative in tagging and nested span policy inside cassandra. For example, this makes annotations, not sub-spans. I think this is a good start and would be useful in zipkin tuning

codefromthecrypt · 2017-05-12T10:22:49Z

instrumentation/cassandra/README.md

+
+// By default, B3 style is used, so instrumented clients do something like this
+Map<String, ByteBuffer> payload = new LinkedHashMap<>();
+payload.set("X-B3-TraceId", byteBuffer("463ac35c9f6413ad"));


note that brave allows you to change the propagation format. I intentionally decided to not go with the finagle binary encoding as most people don't understand what that is. B3 is reasonably well documented, so a better default IMHO.

codefromthecrypt · 2017-05-13T12:27:17Z

buffed the CassandraDriverParser, so it looks prettier and accepts Tagger not Span

codefromthecrypt · 2017-05-13T12:28:41Z

I'm going to rename CassandraDriverX to CassandraClientX in a separate commit. @llinder etc let me know if you disagree with this choice.

codefromthecrypt · 2017-05-13T12:41:12Z

OK I think this is good for a first cut. I'll integrate in zipkin and if it works have no more changes planned for the initial impl. We can always add more functionality later (and likely will need to)

codefromthecrypt · 2017-05-15T02:38:57Z

updated to use the SpanCustomizer type

codefromthecrypt · 2017-05-15T07:44:42Z

I'm not going to merge this until some folks savvy with cassandra have a chance to play around with it.

This contains tracing instrumentation for [Cassandra](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/tracing/Tracing.java) and the [DataStax Java Driver](https://github.com/datastax/java-driver). `brave.cassandra.Tracing` extracts trace state from the custom payload of incoming requests. How long each request takes, each suboperation, and relevant tags like the session ID are reported to Zipkin. `brave.cassandra.driver.TracingSession` tracks the client-side of cassandra and adds trace context to the custom payload of outgoing requests. If server integration is in place, cassandra will contribute data to these RPC spans.

codefromthecrypt · 2017-05-15T09:57:11Z

ps considering the scope of this includes 3 modules, it might make more sense to bump this to a separate repository brave-cassandra. Any thoughts?

devinsba · 2017-05-15T19:07:08Z

👍 to new repo if there will be a bunch of churn on that. Not sure what the cadence is for the cassandra project

llinder · 2017-05-15T21:07:07Z

Separate repository sounds good to me.

llinder · 2017-05-15T21:14:47Z

Looks like a solid start. I will give this a go in one of our services in pre-production as soon as I can. Just want to finish up the Ratpack work first.

michaelsembwever · 2017-05-16T04:57:14Z

instrumentation/cassandra-driver/src/main/java/brave/cassandra/driver/TracingSession.java

+    if (!span.isNoop()) parser.request(statement, span.kind(CLIENT));
+
+    // o.a.c.tracing.Tracing.newSession must use the same propagation format
+    if (version.compareTo(ProtocolVersion.V4) >= 0) {


does it break the request if outgoingPayload is added to it against a server that doesn't support it (ie < V4) ?
(if not, then why not just add it anyway…)

you'd know better than me :) To test this though, I suppose we could add an invoker test that uses an older version of cassandra..

michaelsembwever · 2017-05-16T05:01:04Z

instrumentation/cassandra-driver/src/main/java/brave/cassandra/driver/TracingSession.java

+      span.finish();
+      throw e;
+    }
+    Futures.addCallback(result, new FutureCallback<ResultSet>() {


have we any idea on the impact adding all the callbacks will cause.
EG in a client that processes 10k+ requests per second this could cause trouble, and a suitable load-shedding mechanism be appropriate.

heh actually looking at the code, this is a bit silly.. I am checking span.isNoop for each callback. Will buff this out and move the conditional in front. I don't think we'd want to sample 10K spans/second, so this should take care of that concern.

Anyway, you highlight something which is that we don't yet have benchmarks on this for normal vs instrumented case.

codefromthecrypt · 2017-05-16T06:36:28Z

+public final class CassandraTraceKeys { it's interesting that you use final class here, as appropriate, yet not on the other class. i would not presume that the other classes are designed for being subclassed (which are in fact broadening the API being published beyond what you're probably intending it to be.)

This is just a habit for constant files, i wouldn't read too much into that. If someone were subclassing, they could I guess make a subclass of TraceKeys.. wasn't thinking about that. The other class is subclassable mainly as it seems very likely to need it. I mentioned some notes in the type doc about configuration maybe being something not awesome with system properties. Another thing is how subordinate work is recorded.. right now, for simplicity and also to keep traces from getting too "bottom heavy", I put annotations instead of child spans. Subclassing would allow someone to override that, and since it is likely they'll need to override the construction of the type anyway (bc it is literally new'ed) seemed to fit. I suspect this code will flex a bit before final, but there were my thoughts going into it..

codefromthecrypt · 2017-05-16T06:51:35Z

Tracing in Cassandra is actively maintained, ex there are some notes about things that work in 3.11. I suspect it will actually be the testing and focus concerns that make this more sensible as a separate repo beyond just the several modules reason. For example, we will likely need a matrix build at some point albeit via maven invoker or docker. This is easier as a separate repo. On focus, there is a lot of activity in brave unrelated to cassandra. Another repo could keep the spam level down a bit.

…

On 16 May 2017 3:07 am, "Brian Devins" ***@***.***> wrote: 👍 to new repo if there will be a bunch of churn on that. Not sure what the cadence is for the cassandra project — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61yOot5j9_WvwwFc7paLgVoL93Ljgks5r6KJcgaJpZM4NZGpK> .

michaelsembwever · 2017-05-16T09:50:14Z

instrumentation/cassandra/src/main/java/brave/cassandra/Tracing.java

+    ZipkinTraceState state = (ZipkinTraceState) get();
+    if (state != null) state.incoming.finish();
+  }
+


how come you didn't also override public void doneWithLocalSession(TraceState) ?

possibly an oversight?

Or public TraceState initializeFromMessage(final MessageIn<?> message) ?
Or public Map<String, byte[]> getTraceHeaders()

(by not implementing outbound tracing, you won't be getting the tracing across the nodes within the cluster)

honestly I only implemented barebones to get things off the ground. Notably there's a lot of moving parts just in basics, so I was hoping to get something together that people can contribute to to add more features.

michaelsembwever · 2017-05-16T09:53:18Z

instrumentation/cassandra/src/main/java/brave/cassandra/Tracing.java

+
+  @Override protected final TraceState newTraceState(InetAddress coordinator, UUID sessionId,
+      TraceType traceType) {
+    throw new AssertionError();


why is this an AssertionError?

because this method shouldn't be called because we override what calls it. That said, if the upstream code drifts that could cease to be the case.

codefromthecrypt · 2017-05-16T09:53:54Z

I've moved all this code here for future updates. feel free to adjust anything! https://github.com/openzipkin/brave-cassandra

codefromthecrypt · 2017-05-16T10:08:02Z

added openzipkin/brave-cassandra#1 to not lose this point

codefromthecrypt · 2017-05-16T10:21:08Z

ok! the brave-cassandra repo is in business. For example, you can grab the snapshot cassandra jar here http://oss.jfrog.org/oss-snapshot-local/io/zipkin/brave/cassandra/brave-instrumentation-cassandra/0.0.1-SNAPSHOT/

Thanks for the review folks, and please direct all future things here, por favor! https://github.com/openzipkin/brave-cassandra

codefromthecrypt commented May 12, 2017

View reviewed changes

codefromthecrypt force-pushed the cassandra branch from e79581a to 1f18ddb Compare May 12, 2017 10:34

codefromthecrypt mentioned this pull request May 12, 2017

collaboration on brave-instrumentation-cassandra thelastpickle/cassandra-zipkin-tracing#6

Open

codefromthecrypt force-pushed the cassandra branch from 1f18ddb to 4df48db Compare May 12, 2017 10:45

codefromthecrypt changed the title ~~Adds Cassandra server instrumentation~~ Adds Cassandra client and server integration May 13, 2017

codefromthecrypt force-pushed the cassandra branch 7 times, most recently from a20c40d to e6e3cc5 Compare May 13, 2017 12:26

codefromthecrypt force-pushed the cassandra branch from e6e3cc5 to f9fd710 Compare May 13, 2017 12:39

codefromthecrypt force-pushed the cassandra branch 3 times, most recently from 73fa549 to 0f3eca9 Compare May 15, 2017 02:38

codefromthecrypt force-pushed the cassandra branch 2 times, most recently from 3ee0eba to 8f0a91a Compare May 15, 2017 07:25

codefromthecrypt force-pushed the cassandra branch from 8f0a91a to 00f77e0 Compare May 15, 2017 09:06

Adrian Cole added 2 commits May 15, 2017 17:06

Renames type from CassandraDriver to CassandraClient

00f77e0

michaelsembwever reviewed May 16, 2017

View reviewed changes

michaelsembwever approved these changes May 16, 2017

View reviewed changes

codefromthecrypt closed this in openzipkin/brave-cassandra@4556d02 May 16, 2017

michaelsembwever reviewed May 16, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Cassandra client and server integration #414

Adds Cassandra client and server integration #414

codefromthecrypt commented May 12, 2017 •

edited

Loading

codefromthecrypt commented May 12, 2017

codefromthecrypt May 12, 2017

codefromthecrypt commented May 13, 2017

codefromthecrypt commented May 13, 2017

codefromthecrypt commented May 13, 2017

codefromthecrypt commented May 15, 2017

codefromthecrypt commented May 15, 2017

codefromthecrypt commented May 15, 2017

devinsba commented May 15, 2017

llinder commented May 15, 2017

llinder commented May 15, 2017

michaelsembwever May 16, 2017 •

edited

Loading

codefromthecrypt May 16, 2017

michaelsembwever May 16, 2017

codefromthecrypt May 16, 2017

codefromthecrypt commented May 16, 2017 via email

codefromthecrypt commented May 16, 2017 via email

michaelsembwever May 16, 2017

codefromthecrypt May 16, 2017

michaelsembwever May 16, 2017

codefromthecrypt May 16, 2017

michaelsembwever May 16, 2017

codefromthecrypt May 16, 2017

codefromthecrypt commented May 16, 2017

codefromthecrypt commented May 16, 2017 via email

codefromthecrypt commented May 16, 2017

Adds Cassandra client and server integration #414

Adds Cassandra client and server integration #414

Conversation

codefromthecrypt commented May 12, 2017 • edited Loading

codefromthecrypt commented May 12, 2017

Choose a reason for hiding this comment

codefromthecrypt commented May 13, 2017

codefromthecrypt commented May 13, 2017

codefromthecrypt commented May 13, 2017

codefromthecrypt commented May 15, 2017

codefromthecrypt commented May 15, 2017

codefromthecrypt commented May 15, 2017

devinsba commented May 15, 2017

llinder commented May 15, 2017

llinder commented May 15, 2017

michaelsembwever May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented May 16, 2017 via email

codefromthecrypt commented May 16, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented May 16, 2017

codefromthecrypt commented May 16, 2017 via email

codefromthecrypt commented May 16, 2017

codefromthecrypt commented May 12, 2017 •

edited

Loading

michaelsembwever May 16, 2017 •

edited

Loading