All spans of a trace share sampling state #377

yurishkuro · 2019-08-26T22:44:28Z

Replaces #372

Part of solution for #379

Signed-off-by: Joe Farro <[email protected]>

Signed-off-by: Yuri Shkuro <[email protected]>

codecov · 2019-08-27T19:54:29Z

Codecov Report

Merging #377 into master will decrease coverage by 0.46%.
The diff coverage is 99.53%.

@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
- Coverage   99.01%   98.54%   -0.47%     
==========================================
  Files          44       48       +4     
  Lines        1721     1853     +132     
  Branches      337      352      +15     
==========================================
+ Hits         1704     1826     +122     
- Misses         17       27      +10

Impacted Files	Coverage Δ
src/configuration.js	`98.95% <100%> (ø)`	⬆️
src/span.js	`100% <100%> (ø)`	⬆️
src/span_context.js	`92.74% <100%> (-7.26%)`	⬇️
src/samplers/v2/sampling_state.js	`100% <100%> (ø)`
src/samplers/constants.js	`100% <100%> (ø)`
src/tracer.js	`100% <100%> (ø)`	⬆️
src/samplers/v2/base.js	`100% <100%> (ø)`
src/samplers/_adapt_sampler.js	`97.14% <97.14%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5da0f31...ecf631e. Read the comment docs.

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2019-08-27T23:46:04Z

package.json

@@ -68,7 +68,7 @@
    "precommit": "lint-staged",
    "test": "make test",
    "test-dist": "mocha dist/test/ dist/test/baggage/ dist/test/throttler/ dist/test/samplers/ dist/test/metrics/",
-    "test-all": "npm run test-core && npm run test-samplers && npm run test-crossdock && npm run test-baggage && npm run test-throttler && npm run test-prom-metrics",
+    "test-all": "npm run test-core && npm run test-samplers && npm run test-baggage && npm run test-throttler && npm run test-prom-metrics && npm run test-crossdock",


moved crossdock test in the last position

yurishkuro · 2019-08-27T23:46:30Z

src/configuration.js

@@ -224,7 +224,7 @@ export default class Configuration {
    }

    if (options.logger) {
-      options.logger.info(`Initializing Jaeger Tracer with ${reporter.name()} and ${sampler.name()}`);
+      options.logger.info(`Initializing Jaeger Tracer with ${reporter} and ${sampler}`);


don't need to rely on name() function, toString should work.

Signed-off-by: Yuri Shkuro <[email protected]>

vprithvi

Need to go through again

vprithvi · 2019-08-28T21:21:23Z

.vscode/launch.json

+      "args": ["--timeout", "999999", "--compilers", "js:babel-core/register", "--colors", "${file}"],
+      "console": "integratedTerminal",
+      "internalConsoleOptions": "neverOpen",
+      // "runtimeVersion": "10",


nit: invalid JSON

I think it's fine, actually. It's not a real JSON file. If you open VSCode settings as JSON, it has other comments, like

// Place your settings in this file to overwrite the default settings

vprithvi · 2019-08-28T21:22:52Z

src/_flow/sampler.js

+/**
+ * Sampler methods return SamplingDecision struct.
+ */
+declare type SamplingDecision = {


Perhaps we should call this SamplingResult because it contains information in addition to the decision (0/1) ?

I think "decision" is a more accurate term. It does not mean that it's a primitive yes/no value (cf. Supreme Court decision - lots of additional details), yet it's a decision that is not yet implemented, whereas "result" would be the consequence of implementing that decision.

vprithvi · 2019-08-28T21:37:30Z

src/samplers/_adapt_sampler.js

+  }
+
+  onSetTag(span: Span, key: string, value: any): SamplingDecision {
+    return { sample: false, retryable: true, tags: null };


I think a comment with some reasoning would be useful when we look at this later

I have them in the class comment.

vprithvi · 2019-08-28T21:39:35Z

src/samplers/v2/base.js

+
+let _instanceId = 0;
+
+export default class BaseSamplerV2 implements Sampler {


What does this do (Is it vestigial) ?
I see an import on https://github.com/jaegertracing/jaeger-client-node/pull/377/files#diff-3ac550bfade28896bbd1273d1a26b74dR16

at this point it's not used, until we resolve the babel issue with inheritance. I could remove it, it will still be in the PR history.

I think we should remove it if we are not using it

vprithvi · 2019-08-28T21:41:41Z

src/samplers/v2/sampling_state.js

+  _final: boolean = false;
+
+  // Span ID of the local root span, i.e. the first span in this process for this trace.
+  _localRootSpanIdStr: ?string;


Should we rename this to firstInProcessSpanIdStr? (go client has a var called firstInProcess...)

I think 'local root' is a good name, and I've seen it used in other places (zipkin, OC).

vprithvi · 2019-08-28T21:46:52Z

src/samplers/v2/sampling_state.js

+   *
+   * This field exists to help distinguish between when a span can have a properly
+   * correlated operation name -> sampling rate mapping, and when it cannot.
+   * Adaptive sampling uses the operation name of a span to correlate it with


Should the part about adaptive sampling live elsewhere?

it will, remember this is a change that aims to keep status quo.

vprithvi · 2019-08-28T21:48:16Z

src/samplers/v2/sampling_state.js

+   * then adaptive sampling cannot associate the operation name with the proper sampling rate.
+   * In order to correct this we allow a span to be written to, so that we can re-sample
+   * it in the case that an operation name is set after span creation. Situations
+   * where a span context's sampling decision is finalized include:


Are all of these situations still accurate?

as of this diff, yes

vprithvi · 2019-08-28T21:51:06Z

src/span_context.js

+    const _childId = idIsStr ? null : childId;
+    const _childIdStr = idIsStr ? childId : null;
+    return new SpanContext(
+      this._traceId, // traceId


What is the purpose of these comments?

I find it hard to read without them, especially then values don't match the argument meaning, as in this._spanId, // parentId

Perhaps we should improve this constructor (in a different diff?)

Signed-off-by: Yuri Shkuro <[email protected]>

vprithvi · 2019-09-05T15:04:17Z

src/tracer.js

-
-      ctx.traceId = randomId;
-      ctx.spanId = randomId;
-      ctx.parentId = null;


We have a test in the adaptor that explicitly checks that parentId is null (as opposed to undefined) for a newly created span - do you think this behavior is expected and relied upon elsewhere else?

we should restore it, probably assign null explicitly in the constructor

tiffon and others added 14 commits August 26, 2019 18:18

All spans of a trace share sampling state

c82c4d7

Signed-off-by: Joe Farro <[email protected]>

Un-move code, new methods are private, misc tweaks

c31fa61

Signed-off-by: Joe Farro <[email protected]>

Extend the timeout for a flaky udp test

fab133b

Signed-off-by: Joe Farro <[email protected]>

Bump timeout for slow tests, consistent CI distro

441b5df

Signed-off-by: Joe Farro <[email protected]>

Undo moving of functions

252234b

Signed-off-by: Yuri Shkuro <[email protected]>

Use base class for V2 samplers

ff9cfe8

Signed-off-by: Yuri Shkuro <[email protected]>

Return SamplingDecision from new samplers

c60bae1

Signed-off-by: Yuri Shkuro <[email protected]>

Fix rebase issues

2c92d9c

Signed-off-by: Yuri Shkuro <[email protected]>

Simplify v1 adapter

7a19f4a

Signed-off-by: Yuri Shkuro <[email protected]>

Cleaning up

22ed6aa

Signed-off-by: Yuri Shkuro <[email protected]>

Fix test/span.js

fc618bd

Signed-off-by: Yuri Shkuro <[email protected]>

Fix test/tracer.js

7a61c5f

Signed-off-by: Yuri Shkuro <[email protected]>

Fix more tests

c74ffba

Signed-off-by: Yuri Shkuro <[email protected]>

Undo blank line

456fe75

Signed-off-by: Yuri Shkuro <[email protected]>

Yuri Shkuro added 5 commits August 27, 2019 16:03

Prettify

5412247

Signed-off-by: Yuri Shkuro <[email protected]>

Fix flow error

4709f28

Signed-off-by: Yuri Shkuro <[email protected]>

Add more tests

9c45195

Signed-off-by: Yuri Shkuro <[email protected]>

Clean-up

0a274c4

Signed-off-by: Yuri Shkuro <[email protected]>

Restore previous tests

56abd5c

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro commented Aug 27, 2019

View reviewed changes

Yuri Shkuro added 8 commits August 27, 2019 20:00

Cleanup tests

c832b14

Signed-off-by: Yuri Shkuro <[email protected]>

Use legacy const sampler

7fd9beb

Signed-off-by: Yuri Shkuro <[email protected]>

Fix imports

9fa50bf

Signed-off-by: Yuri Shkuro <[email protected]>

Use legacy ConstSampler

e011c99

Signed-off-by: Yuri Shkuro <[email protected]>

Fix param order

d2616b3

Signed-off-by: Yuri Shkuro <[email protected]>

Remove flow again

4169b29

Signed-off-by: Yuri Shkuro <[email protected]>

Remove v2 ConstSampler for now; add tests

8a4b5cb

Signed-off-by: Yuri Shkuro <[email protected]>

Fix flow errors

924712a

Signed-off-by: Yuri Shkuro <[email protected]>

Remove inheritance

2e1cfd5

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro force-pushed the shared-sampling-state branch from 6b080a9 to 2e1cfd5 Compare August 28, 2019 20:34

vprithvi reviewed Aug 28, 2019

View reviewed changes

Yuri Shkuro added 4 commits August 28, 2019 18:53

Add comments;

e5ac5fb

Signed-off-by: Yuri Shkuro <[email protected]>

Add firehose flag test

669dbff

Signed-off-by: Yuri Shkuro <[email protected]>

Add tests for adapter

cbc4b2f

Signed-off-by: Yuri Shkuro <[email protected]>

Fix flow

75e6d4a

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro mentioned this pull request Aug 29, 2019

All spans of a trace share sampling state #372

Closed

Add toString test

ecf631e

Signed-off-by: Yuri Shkuro <[email protected]>

vprithvi approved these changes Aug 29, 2019

View reviewed changes

yurishkuro merged commit 0fae741 into jaegertracing:master Aug 29, 2019

vprithvi reviewed Sep 5, 2019

View reviewed changes

This was referenced Sep 5, 2019

Parent should be null, not undefined #386

Closed

Adaptive Sampling jaegertracing/jaeger#365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All spans of a trace share sampling state #377

All spans of a trace share sampling state #377

yurishkuro commented Aug 26, 2019 •

edited

Loading

codecov bot commented Aug 27, 2019 •

edited

Loading

yurishkuro Aug 27, 2019

yurishkuro Aug 27, 2019

vprithvi left a comment

vprithvi Aug 28, 2019

yurishkuro Aug 28, 2019

vprithvi Aug 28, 2019

yurishkuro Aug 28, 2019

vprithvi Aug 28, 2019

yurishkuro Aug 28, 2019

vprithvi Aug 28, 2019

yurishkuro Aug 28, 2019

vprithvi Aug 29, 2019

vprithvi Aug 28, 2019

yurishkuro Aug 28, 2019

vprithvi Aug 28, 2019

yurishkuro Aug 28, 2019

vprithvi Aug 28, 2019

yurishkuro Aug 28, 2019

vprithvi Aug 28, 2019

yurishkuro Aug 28, 2019

vprithvi Aug 29, 2019

vprithvi Sep 5, 2019

yurishkuro Sep 5, 2019


		let _instanceId = 0;

		export default class BaseSamplerV2 implements Sampler {

All spans of a trace share sampling state #377

All spans of a trace share sampling state #377

Conversation

yurishkuro commented Aug 26, 2019 • edited Loading

codecov bot commented Aug 27, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vprithvi left a 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

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

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

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

yurishkuro commented Aug 26, 2019 •

edited

Loading

codecov bot commented Aug 27, 2019 •

edited

Loading