Tracing by protocol7 · Pull Request #136 · spotify/folsom

protocol7 · 2019-06-15T09:27:32Z

This PR is based on top of #135 so please review that first.

coveralls · 2019-06-15T09:37:20Z

Coverage remained the same at ?% when pulling 4058b7a on tracing into 84a6104 on master.

protocol7 · 2019-06-15T11:26:01Z

@dmichel1 tracing PR as mentioned, would be awesome if you could take a look

dmichel1

left a few comments.

can you add how to use this to the readme as well?

exciting!

dmichel1 · 2019-06-16T13:21:53Z

folsom-opencensus/src/main/java/com/spotify/folsom/opencensus/OpenCensusTracer.java

+    requireNonNull(future);
+    requireNonNull(operation);
+
+    final io.opencensus.trace.Tracer tracer = Tracing.getTracer();


typically this is a static variable - https://github.com/census-instrumentation/opencensus-java/blob/master/examples/src/main/java/io/opencensus/examples/trace/MultiSpansTracing.java#L31

Ah cool, will fix.

dmichel1 · 2019-06-16T13:28:21Z

folsom-opencensus/src/main/java/com/spotify/folsom/opencensus/OpenCensusTracer.java

+    ocSpan.putAttribute("component", stringAttributeValue("folsom"));
+    ocSpan.putAttribute("peer.service", stringAttributeValue("memcache"));
+    ocSpan.putAttribute("operation", stringAttributeValue(operation));
+    if (key != null) {


should adding the key as an annotation be configurable? I'm thinking there might be scenarios where key is "sensitive."

I was thinking about this, which is why I opted not to include the value. But perhaps this is worth considering also with the key.

knowing what key is potentially causing an issue could definitely be valuable during troubleshooting so I can see the benefit of leaving it here/enabled by default...

what about adding another attribute for the size of the payload being sent to memcached? that could also be valuable during troubleshooting

Great suggestions! Pushed an update for addressing both of these. There's now a builder to configure if you want keys/values included in traces. Also added value size.

dmichel1 · 2019-06-16T13:44:57Z

folsom-opencensus/src/main/java/com/spotify/folsom/opencensus/OpenCensusTracer.java

+
+    final io.opencensus.trace.Tracer tracer = Tracing.getTracer();
+
+    final io.opencensus.trace.Span ocSpan =


this is the trickiest part of tracing especially with async operations happening...

with this implicit context propagation traces spans might not be linked when they go from a say an apollo thread pool -> memcached thread pool.

if this is indeed the case here, the span can be added as parameter to these methods and passed in with the memcached operation. This would then become tracer.spanBuilderWithExplicitParent()

https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/Span.md#how-span-interacts-with-context

Yeah, this assumes that the context is propagated to the calling thread/gRPC Context. Once in Folsom-land, this should works as expected. But, this would be equally true for tracing in say Bigtable/Spanner, right (I've only looked at the Spanner client and it seems to use the same pattern as here)? If so, I believe we should make sure we propagate the tracing context correctly at least out of the box in Apollo, is this the case today?

we should be propagating the context in apollo correctly as the traces to any gRPC based service (bigtable/pubsub/spanner) are linked already. have you tried testing this code with an apollo services yet?

I haven't yet, but got a service hopefully lined up

I think this pattern is generally used for many libraries, Bigtable does it similarly. It may be useful to allow passing in the parent span instead of solely relying on getCurrentSpan - that way if the calling code is doing something unusual there's a way to keep the context correctly linked.

@hexedpackets do you have a reference handy to how this is done in Bigtable, would be curious as to how they opted to design it? I tried to find, but not sure I'm looking in the right place.

For now, I would not want to change the Folsom API in order to support tracing, but if we see more requests from users on setting explicit parent spans I think we should look closer at how to do this in the future.

That makes sense, I think the use case of needing to pass in the parent span should be pretty rare.

Bigtable creates a span when the operation is instantiated and then closes it in a callback after the operation ends. I don't think this is passed around, but various functions in the class add tags and annotations to the span when they're run.

dmichel1 · 2019-06-16T13:46:44Z

folsom/src/main/java/com/spotify/folsom/client/ascii/DefaultAsciiMemcacheClient.java

    SetRequest request = SetRequest.casSet(byteKey, valueBytes, ttl, cas);
    CompletionStage<MemcacheStatus> future = rawMemcacheClient.send(request);
    metrics.measureSetFuture(future);
+    tracer.span("folsom.set", future, "set", key);


If we need to do explicit propagation this would have to look something like...

public CompletionStage<MemcacheStatus> set(String key, V value, int ttl, long cas, Span span) { ... tracer.span("folsom.set", future, "set", key, span);

I'd like to avoid polluting the Folsom API if at all possible :)

👍 - agreed

protocol7 · 2019-06-16T16:59:27Z

Fix pushed which addresses comments so far, including an updated README

dmichel1

Couple more comments and then let's try this out.

dmichel1 · 2019-06-17T13:11:11Z

README.md

    </dependency>

-If you want to use one of the metric libraries, make sure you use the same version as the main artifact.
+    <!-- optional if you want to expose folsom metrics with yammer -->


s/folsom metrics with yammer/traces with OpenCensus/

dmichel1 · 2019-06-17T13:14:36Z

folsom-opencensus/src/main/java/com/spotify/folsom/opencensus/OpenCensusSpan.java

+    if (includeValues) {
+      span.putAttribute("value_hex", stringAttributeValue(HEX.encode(value)));
+    }
+    span.putAttribute("value_size", longAttributeValue(value.length));


include the unit here. e.g. value_size_bytes

dmichel1 · 2019-06-17T13:20:44Z

folsom-opencensus/src/main/java/com/spotify/folsom/opencensus/OpenCensus.java

+public class OpenCensus {
+
+  private static final boolean DEFAULT_INCLUDE_KEYS = true;
+  private static final boolean DEFAULT_INCLUDE_VALUES = false;


I'm a bit concerned about someone enabling the value attribute. It could be a really big payload. I think since folks have the ability to see the key if they really care about the value they can fetch it themselves.

I would be in favor of removing the value attribute entirely.

I think it's fine as long as this is default to off, but I'll add a note about this in the documentation.

This add support for tracing memcache operations, including an implementation for OpenCensus. This change opts to only trace high level operations for the sake of simplicity. It will for example not trace operations to individual nodes (e.g. for delete all operations). As we learn more about tracing needs, we might change this to include more fine-grained operations. The OpenCensus implementation attempts to use the OpenTracing conventions for attributes: https://opentracing.io/specification/conventions/

protocol7 force-pushed the tracing branch 4 times, most recently from e8c5014 to 8f581a9 Compare June 15, 2019 12:10

protocol7 mentioned this pull request Jun 15, 2019

Add tracing #134

Closed

dmichel1 reviewed Jun 16, 2019

View reviewed changes

protocol7 force-pushed the tracing branch from 8f581a9 to e8b254d Compare June 16, 2019 16:56

dmichel1 approved these changes Jun 17, 2019

View reviewed changes

protocol7 force-pushed the tracing branch from e8b254d to 32c377e Compare June 18, 2019 11:36

protocol7 force-pushed the tracing branch from 32c377e to 4058b7a Compare June 18, 2019 11:48

protocol7 merged commit bb78316 into master Jun 18, 2019

protocol7 deleted the tracing branch June 18, 2019 18:38


		final io.opencensus.trace.Tracer tracer = Tracing.getTracer();

		final io.opencensus.trace.Span ocSpan =

Conversation

protocol7 commented Jun 15, 2019

Uh oh!

coveralls commented Jun 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

protocol7 commented Jun 15, 2019

Uh oh!

dmichel1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hexedpackets Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

protocol7 commented Jun 16, 2019

Uh oh!

dmichel1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jun 15, 2019 •

edited

Loading

hexedpackets Jun 19, 2019 •

edited

Loading