Allow using custom Recorder and add option to always record spans #158

drolando · 2020-06-13T02:22:24Z

This will let me implement "firehose mode", similar to what we have in
py_zipkin.

Basically what I need is a way to call 2 different Reporters:

one gets called only when sampled is True
one gets called every time (this is the firehose recorder)
This cannot be accomplished with just one Reporter, because once we're
inside the reporter we've lost any info on whether the trace was sampled
or not.

I can almost do this in zipkin-php, there are only 2 things missing.

if sampled is False, we create a NoopSpan. So I need an extra flag to
inform the tracer that I always want to create a RealSpan
the Recorder is hardcoded inside the Tracer class, so I cannot pass my
own.

Once those 2 things are supported, I can write my own Recorder subclass
that calls the right Reporters as needed with minimal changes to the
core zipkin-php code.

cc @adriancole

codefromthecrypt · 2020-06-13T02:36:07Z

in Brave we solve this with sampledLocal internal flag

https://github.com/openzipkin/brave/blob/169d93ac71fce759fb872636576d539c6c3b5123/brave/src/main/java/brave/propagation/SamplingFlags.java#L89-L102

then the zipkin "recorder" would look at if it is supposed to act as default (only send ones remote sampled), or wildcard send (alwaysSampleLocal)
https://github.com/openzipkin/zipkin-reporter-java/blob/master/brave/src/main/java/zipkin2/reporter/brave/ZipkinSpanHandler.java#L147

Note: something slightly different what you are doing vs alwaysSampleLocal used in secondary sampling.. in secondary sampling how netflix use it, they add a comma-separated tag "sampled_keys" so that their firehose proxy can decide what to do vs decide in-process. Ex. if that tag includes "b3" they know it was sampled because of b3 vs wildcard or some overlay.

https://github.com/openzipkin-contrib/zipkin-secondary-sampling/blob/master/docs/design.md#the-sampled_keys-tag
https://github.com/openzipkin-contrib/zipkin-secondary-sampling/blob/master/brave/src/test/java/brave/secondary_sampling/BasicUsageTest.java#L86-L90

If making the decision in-process you don't need to add a tag, just route it to another sender based on logic you deploy.

drolando · 2020-06-13T06:18:55Z

Doing something similar here would mean adding a new sampledLocal flag to TraceContext for each span. Then in the reporter I can loop through them and send all the ones that have sampledLocal=true to firehose, correct?

drolando · 2020-06-13T06:20:02Z

And I see you have the same alwaysReportSpans flag too

codefromthecrypt · 2020-06-13T08:25:18Z

yeap

sampledlocal is true when sampled is true or if alwaysReportSpans is set. (later you can get fancy and conditionally set sampledLocal based on http headers)

alwaysReportSpans is a tracer-scoped flag where sampledLocal is a span/context scoped flag

jcchavezs · 2020-06-15T08:47:38Z

@drolando thanks for this. I am reluctant to expose the Recorder to the user, mostly because I always saw it as an internal type and making it customizable would create some weird API surface (starting by the fact that we pass the reporter as a parameter already and it is supposed to be included in the recorder aswell). I'd prefer the approach @adriancole suggests by using the sampledLocal flag.

@adriancole I got a question, to summarize what we could do is:

Include the Context Flags or at leas the sampledLocal in the MutableSpan (so the reporter has access to sampledLocal, not sure which option is the best)
Create the SpanHandler interface whose default class only forwards the remote sampled spans (notice this can enable better things like only forward remote sampled spans with an error or certain duration)
Let the Reporter to decide how to multiplex the spans based on the sampledLocal flags?

codefromthecrypt · 2020-06-17T13:34:18Z

tests/Unit/TracingBuilderTest.php

+        $span->finish();
+
+        $tracer->flush();
+        $spans = $reporter->flush();


I don't fully understand why we are using another word. Ex alwaysReportSpans would make sense to have reporter report. alwaysEmitSpans would makes sense to have emitter emit :)

here's a related post on naming the TL;DR; being that we make enough words at this point https://publicobject.com/2020/06/06/synonyms-are-bad/

oh yeah that's a better name

codefromthecrypt

I think the use case is valid.. made a comment about naming (bikeshed but I think worth thinking for a sec)

regardless, probably copying some of the doc from here might help!

https://github.com/openzipkin/zipkin-reporter-java/blob/master/brave/src/main/java/zipkin2/reporter/brave/ZipkinSpanHandler.java#L109-L127

drolando · 2020-06-24T00:58:31Z

@jcchavezs any comment on this? Does it look good?

jcchavezs · 2020-06-24T03:46:14Z

Sorry for the delay @drolando. It looks good to me, only one note: `isSampled` in the recording span would always be Boolean by the time it is reported. No need to be nullable. Also, do we need some docs or notes around what alwayaRecordSpans would cause? Like I know Tracing builder does not have docs for methods but maybe this is a good one to start with? Other than that I like it!

…

On Wed, 24 Jun 2020, 02:58 Daniele, ***@***.***> wrote: @jcchavezs <https://github.com/jcchavezs> any comment on this? Does it look good? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYASOBFMQM4NLKFAKEQLRYFFUHANCNFSM4N4ZANAA> .

codefromthecrypt · 2020-06-24T04:06:16Z

src/Zipkin/Recording/Span.php

@@ -156,7 +163,8 @@ public static function createFromContext(TraceContext $context, Endpoint $localE
            $context->getSpanId(),
            $context->isDebug(),
            $context->isShared(),
-            $localEndpoint
+            $localEndpoint,
+            $context->isSampled()


ps one slight thing here is that in brave the "handler" has 2 arguments. since it has the context as one of the args, we don't need to also copy isSampled to the span.

@Override public boolean end(TraceContext context, MutableSpan span, Cause cause) { if (!alwaysReportSpans && !Boolean.TRUE.equals(context.sampled())) return true; --snip--

I'm not saying to change this, just mentioning a reference. it is fine to also copy it to the span, just it should be immutable (appears to be the case!)

drolando · 2020-07-01T23:02:00Z

isSampled in the recording span would always be Boolean by the time it is reported. No need to be nullable.

This isn't always true, at least in tests. If I create the context with TraceContext::createAsRoot(DefaultSamplingFlags::createAsEmpty()); then isSampled is null.

jcchavezs · 2020-07-03T18:20:02Z

This isn't always true, at least in tests. If I create the context with TraceContext::createAsRoot(DefaultSamplingFlags::createAsEmpty()); then isSampled is null.

Yes but when making it a span then the sampling decision happens. Anyways I am going to merge this and then investigate that and rebase onto #163.

EDIT: I need some input. As I mentioned when we turn a context with null sampled into span, we force to make the sampling decision and theoretically it will never happen that the reporter receives an unsampled MutableSpan. However that needs the tests for MutableSpan to be aware of this (hence not using null sampled contexts). This can be fixed with some documentation or just accept null sampled (which is not too terrible), how do you feel about that @adriancole ?

jcchavezs · 2020-07-03T18:20:47Z

src/Zipkin/Recording/Span.php

    private function __construct(
        string $traceId,
        ?string $parentId,
        string $spanId,
        bool $debug,
        bool $shared,
-        Endpoint $localEndpoint
+        Endpoint $localEndpoint,
+        ?bool $isSampled = false


I will also move this before endpoint as we changed the order already.

jcchavezs · 2020-07-03T18:22:11Z

tests/Unit/TracingBuilderTest.php

@@ -85,4 +86,58 @@ private function randomBool()
    {
        return (bool) mt_rand(0, 1);
    }
+
+    public function testAlwaysEmitSpans()


I will also change this test into alwaysReportSpans.

jcchavezs · 2020-07-03T18:22:57Z

Thanks for the great work @drolando !

codefromthecrypt · 2020-07-04T00:13:24Z

on the sampled null part. Indeed currently in brave a span must have a sampling decision, but since the context is independent of the span, and it could nave a null decision, then we have to allow it null. At some point we wanted to have "incognito mode" or "pass through" where you don't interfere with anything in the context, but let it be passed across. Especially in that case it could pass around null , but even now it is possible even if not normal.

jcchavezs · 2020-07-05T11:28:45Z

src/Zipkin/Recording/Span.php

+     */
+    public function isSampled(): bool
+    {
+        return $this->isSampled === true;


Got a question here @adriancole @drolando: what we are saying here is whether this is sampled or not (and then dispatch the span to different reporters accordingly) but what about debug? if something is sampled=false but debug=true then we will dispatch it to the local sampled reporter, isn't?

Probably it is better we change this into shouldRecord which also checks the debug flag?

Giving a second thought, I think it is responsability of the user to decide what to do when sampled=false and debug=true.

codefromthecrypt · 2020-07-05T23:28:19Z

sampled explicitly false and debug true is a nonsense value..the only nonsense choice so I wouldnt put much thought into it. depends on the order set. for example we set sampled when debug is set (debug is effectively a boosted sample decision) here's a similar chat about it being a nonsense value openzipkin/b3-propagation#31 (comment)

…

On Sun, Jul 5, 2020, 10:35 PM José Carlos Chávez ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Zipkin/Recording/Span.php <#158 (comment)>: > @@ -225,6 +233,14 @@ public function setRemoteEndpoint(Endpoint $remoteEndpoint): void $this->remoteEndpoint = $remoteEndpoint; } + /** + * @return bool + */ + public function isSampled(): bool + { + return $this->isSampled === true; Giving a second thought, I think it is responsability of the user to decide what to do when sampled=false and debug=true. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPVV3SPCODYRZ2Q44NUZDR2CFTDANCNFSM4N4ZANAA> .

drolando force-pushed the support_firehose branch 3 times, most recently from 9271bde to dabaf31 Compare June 15, 2020 22:16

codefromthecrypt reviewed Jun 17, 2020

View reviewed changes

codefromthecrypt approved these changes Jun 17, 2020

View reviewed changes

codefromthecrypt reviewed Jun 24, 2020

View reviewed changes

drolando added 3 commits July 1, 2020 15:01

Add option to always emit spans even if sampled is false

60d9d2b

Rename alwaysEmitSpans to alwaysReportSpans

58cc5e8

Add docstring to TracingBuilder new function

98adc64

drolando force-pushed the support_firehose branch from b4c1854 to 98adc64 Compare July 1, 2020 22:52

isSampled is null in tests

ea9e7f4

jcchavezs reviewed Jul 3, 2020

View reviewed changes

jcchavezs merged commit b5b5fdd into openzipkin:master Jul 3, 2020

jcchavezs reviewed Jul 5, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using custom Recorder and add option to always record spans #158

Allow using custom Recorder and add option to always record spans #158

drolando commented Jun 13, 2020

codefromthecrypt commented Jun 13, 2020

drolando commented Jun 13, 2020

drolando commented Jun 13, 2020

codefromthecrypt commented Jun 13, 2020

jcchavezs commented Jun 15, 2020

codefromthecrypt Jun 17, 2020

drolando Jun 17, 2020

codefromthecrypt left a comment

drolando commented Jun 24, 2020

jcchavezs commented Jun 24, 2020 via email

codefromthecrypt Jun 24, 2020

codefromthecrypt Jun 24, 2020

drolando commented Jul 1, 2020

jcchavezs commented Jul 3, 2020 •

edited

Loading

jcchavezs Jul 3, 2020

jcchavezs Jul 3, 2020

jcchavezs commented Jul 3, 2020

codefromthecrypt commented Jul 4, 2020

jcchavezs Jul 5, 2020

jcchavezs Jul 5, 2020

jcchavezs Jul 5, 2020

codefromthecrypt commented Jul 5, 2020 via email

Allow using custom Recorder and add option to always record spans #158

Allow using custom Recorder and add option to always record spans #158

Conversation

drolando commented Jun 13, 2020

codefromthecrypt commented Jun 13, 2020

drolando commented Jun 13, 2020

drolando commented Jun 13, 2020

codefromthecrypt commented Jun 13, 2020

jcchavezs commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

drolando commented Jun 24, 2020

jcchavezs commented Jun 24, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drolando commented Jul 1, 2020

jcchavezs commented Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented Jul 3, 2020

codefromthecrypt commented Jul 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Jul 5, 2020 via email

jcchavezs commented Jul 3, 2020 •

edited

Loading