Support delayed sampling #380

yurishkuro · 2019-08-29T21:29:05Z

Which problem is this PR solving?

Support delayed sampling Support delayed sampling #379

Short description of the changes

Sampling finalized mode is no longer early triggered by read operations like injecting context or starting a child span. Instead, sampling may remain unfinalized indefinitely if at least one of the samplers returns retryable=true.
Introducing remote flag in SpanContext that is set when the context has been extracted from a carrier/request. When a child span has a remote context as a parent, the child sampling is finalized immediately, thus preserving the existing behavior of respecting the upstream sampling decision.
Most samplers are upgraded to V2
- Remote and PerOperation samplers are completely switched to V2, because they always delegate to the underlying sampler and isSampled() cannot be adequately delegated to V2 API
- Primitive samplers implement both V1 and V2
- GuaranteedThroughputSampler is kept at V1 because it delegates to primitive ones using V1 API (there's no benefit for it to use V2)
Primitive samplers now return retryable=false from onCreateSpan. If someone wants a deferred sampling behavior, then some samplers should be returning retryable=true.
Adding experimental tag-sampler and priority-sampler

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro · 2019-09-03T15:42:32Z

test/span.js

@@ -244,166 +252,180 @@ describe('span should', () => {
    assert.equal(prevTagLength, span._tags.length, 'The sampling.priority tag should only be set once');
  });

-  describe('adaptive sampling tests for span', () => {


moved to top level test below

src/util.js

vprithvi · 2019-09-03T15:02:15Z

src/samplers/probabilistic_sampler.js

  _samplingRate: number;

  constructor(samplingRate: number) {
+    super('ProbabilisticSampler');


Is calling by name the preferred way of invoking super? It seems pretty fragile

it simply passes the "name" that is used in toString and uniqueName() functions.

vprithvi · 2019-09-03T15:04:02Z

src/samplers/probabilistic_sampler.js

@@ -26,6 +28,14 @@ export default class ProbabilisticSampler implements LegacySamplerV1 {
    this._samplingRate = samplingRate;
  }

+  update(samplingRate: number): boolean {
+    if (this._samplingRate == samplingRate) {


Is there an equality function which takes in an epsilon for float comparisons?

not that I know off. I don't think it matters here. If the rate is coming from JSON strategy, then float representation will be the same as long as the string representation is the same.

vprithvi · 2019-09-03T15:06:11Z

src/span_context.js

@@ -26,6 +26,7 @@ export default class SpanContext {
  _baggage: any;
  _debugId: ?string;
  _samplingState: SamplingState;
+  _remote: boolean; // indicates that span context represents a remote parent


Replace with isRemoteParent?

In Java Beans naming we don't add is to the field names. And parent would be incorrect here because it's this span context that is "remote".

src/tracer.js

test/samplers/delayed_sampling.js

vprithvi · 2019-09-03T15:37:30Z

test/samplers/delayed_sampling.js

+  }
+
+  declare type PrioritySamplerState = {
+    samplerFired: Array<boolean>,


How is samplerFired different than retryable?

samplerFired is set once the given sampler returns retryabe=false. I will add a type comment.

vprithvi · 2019-09-03T15:41:01Z

test/samplers/delayed_sampling.js

+      assert.isFalse(span._spanContext.samplingFinalized, 'finalized');
+    });
+
+    it('should sample and finalize created span with tag', () => {


Might be better to nest these under a "matching tag(s)" describe block

test/samplers/delayed_sampling.js

vprithvi · 2019-09-03T15:44:47Z

I think the API looks pretty usable

test/samplers/delayed_sampling.js

Signed-off-by: Yuri Shkuro <ys@uber.com>

src/samplers/experimental/tag_equals_sampler.js

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro · 2019-09-04T02:38:02Z

test/samplers/exp_extend_remote_sampler.js

+        this._sampler = new PrioritySampler([tagSampler, this._sampler]);
+        return true;
+      }
+      return super._updateSampler(strategy);


@vprithvi this is working, but is hacky, because super._updateSampler works in-place, so I have to do tricks with overriding this._sampler multiple times. Also, the tag & priority samplers are replaced every time, which is not necessarily a problem since they are both stateless and we don't run updates frequently.

These trade offs seem reasonable for our use case

yurishkuro · 2019-09-04T02:39:13Z

test/samplers/exp_extend_remote_sampler.js

+
+  it('should parse extended strategy response', function(done) {
+    server.addStrategy('service1', {
+      strategyType: 'extended',


@vprithvi thoughts?

Not a fan of this in the short term bc it would mean that the backend implementation would need to modify the payload from the thrift sampling object (or modify it after it has been converted to JSON).

The easiest way to avoid it is to nest the "classic" response under a property.

just made that change

yurishkuro · 2019-09-04T02:40:45Z

test/samplers/exp_extend_remote_sampler.js

+    remoteSampler.close();
+  });
+
+  it('should parse extended strategy response', function(done) {


as a POC this works, but we probably need to add more tests to ensure that the extended remote sampler can switch back and forth between extended and normal adaptive behavior.

Signed-off-by: Yuri Shkuro <ys@uber.com>

src/samplers/experimental/tag_equals_sampler.js

vprithvi · 2019-09-04T17:22:25Z

test/samplers/exp_extend_remote_sampler.js

+        this._sampler = new PrioritySampler([tagSampler, this._sampler]);
+        return true;
+      }
+      return super._updateSampler(strategy);


These trade offs seem reasonable for our use case

vprithvi · 2019-09-04T17:24:27Z

test/samplers/exp_extend_remote_sampler.js

+
+  it('should parse extended strategy response', function(done) {
+    server.addStrategy('service1', {
+      strategyType: 'extended',


Not a fan of this in the short term bc it would mean that the backend implementation would need to modify the payload from the thrift sampling object (or modify it after it has been converted to JSON).

Signed-off-by: Yuri Shkuro <ys@uber.com>

jaegertracing#380)

Yuri Shkuro added 3 commits August 29, 2019 17:13

Support late sampling decisions

3f74c34

Signed-off-by: Yuri Shkuro <ys@uber.com>

Extract countdown callback into util

683ca38

Signed-off-by: Yuri Shkuro <ys@uber.com>

Add PrioritySampler

784c5f8

Signed-off-by: Yuri Shkuro <ys@uber.com>

This comment has been minimized.

Sign in to view

Yuri Shkuro added 4 commits August 30, 2019 14:15

Remove eager finalizer

eb17f0c

Signed-off-by: Yuri Shkuro <ys@uber.com>

Clean up flwo errors

c4cafd1

Signed-off-by: Yuri Shkuro <ys@uber.com>

Do not use array.fill()

706d57c

Signed-off-by: Yuri Shkuro <ys@uber.com>

Add tests for remote parent

76be657

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro changed the title ~~[WIP] Support delayed sampling~~ Support delayed sampling Aug 30, 2019

yurishkuro changed the title ~~Support delayed sampling~~ [WIP] Support delayed sampling Aug 30, 2019

Yuri Shkuro added 6 commits August 30, 2019 17:39

rename ratelimiting_sampler.js -> rate_limiting_sampler.js

79e3190

Signed-off-by: Yuri Shkuro <ys@uber.com>

Upgrade probabilistic and rate limiting samplers to v2

f74eae1

Signed-off-by: Yuri Shkuro <ys@uber.com>

Upgrade most samplers to V2

f2ee493

Signed-off-by: Yuri Shkuro <ys@uber.com>

Fix flow error

6fe64c0

Signed-off-by: Yuri Shkuro <ys@uber.com>

Cleanup some stuff

5f0f5e1

Signed-off-by: Yuri Shkuro <ys@uber.com>

Test that per-operation-sampler still behaves as before

d077561

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro force-pushed the delayed-sampling branch from a40a9f9 to d077561 Compare September 1, 2019 04:25

yurishkuro commented Sep 3, 2019

View reviewed changes

vprithvi reviewed Sep 3, 2019

View reviewed changes

yurishkuro commented Sep 3, 2019

View reviewed changes

test/samplers/delayed_sampling.js Outdated Show resolved Hide resolved

Yuri Shkuro added 9 commits September 3, 2019 12:18

Address review comments

a1cb4ae

Signed-off-by: Yuri Shkuro <ys@uber.com>

Simplify with inheritance

689652b

Signed-off-by: Yuri Shkuro <ys@uber.com>

Move new samplers to /experimental/

714f55c

Signed-off-by: Yuri Shkuro <ys@uber.com>

Support firehose in tag sampler

6bc8f20

Signed-off-by: Yuri Shkuro <ys@uber.com>

Fix flow issue

cfe6768

Signed-off-by: Yuri Shkuro <ys@uber.com>

Expose loggers

ae7dc12

Signed-off-by: Yuri Shkuro <ys@uber.com>

Extend remote sampler in test

4cd7ce2

Signed-off-by: Yuri Shkuro <ys@uber.com>

Increase code coverage

88436f7

Signed-off-by: Yuri Shkuro <ys@uber.com>

Add more tests

79cdc3d

Signed-off-by: Yuri Shkuro <ys@uber.com>

Yuri Shkuro added 2 commits September 3, 2019 19:20

Fix flow error

352dbcc

Signed-off-by: Yuri Shkuro <ys@uber.com>

Add tests

6241aae

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro commented Sep 4, 2019

View reviewed changes

src/samplers/experimental/tag_equals_sampler.js Outdated Show resolved Hide resolved

More tests

31cdc22

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro commented Sep 4, 2019

View reviewed changes

yurishkuro changed the title ~~[WIP] Support delayed sampling~~ Support delayed sampling Sep 4, 2019

More tests

e10dbdc

Signed-off-by: Yuri Shkuro <ys@uber.com>

vprithvi approved these changes Sep 4, 2019

View reviewed changes

Use 'classicStrategy' property

465599c

Signed-off-by: Yuri Shkuro <ys@uber.com>

vprithvi approved these changes Sep 4, 2019

View reviewed changes

Fix flow error

0454958

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro merged commit ca64265 into jaegertracing:master Sep 4, 2019

yurishkuro deleted the delayed-sampling branch September 4, 2019 20:22

yurishkuro mentioned this pull request Sep 7, 2019

Adaptive Sampling jaegertracing/jaeger#365

Closed

3 tasks

This was referenced Oct 16, 2019

Rename current samplers to LegacyV1 #374

Merged

Do not apply adaptive sampler to child spans #410

Merged

Iuriy-Budnikov pushed a commit to agile-pm/jaeger-client-node that referenced this pull request Sep 25, 2021

chore: rename opentelemetry-basic-tracer -> opentelemetry-tracer-basic (

75a3297

jaegertracing#380)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support delayed sampling #380

Support delayed sampling #380

yurishkuro commented Aug 29, 2019 •

edited

Loading

This comment has been minimized.

yurishkuro Sep 3, 2019

vprithvi Sep 3, 2019

yurishkuro Sep 3, 2019

vprithvi Sep 3, 2019

yurishkuro Sep 3, 2019

vprithvi Sep 3, 2019

yurishkuro Sep 3, 2019

vprithvi Sep 3, 2019

yurishkuro Sep 3, 2019

vprithvi Sep 3, 2019

vprithvi commented Sep 3, 2019

yurishkuro Sep 4, 2019

vprithvi Sep 4, 2019

yurishkuro Sep 4, 2019

vprithvi Sep 4, 2019

yurishkuro Sep 4, 2019

yurishkuro Sep 4, 2019

yurishkuro Sep 4, 2019

vprithvi Sep 4, 2019

vprithvi Sep 4, 2019

Support delayed sampling #380

Support delayed sampling #380

Conversation

yurishkuro commented Aug 29, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

This comment has been minimized.

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

vprithvi commented Sep 3, 2019

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 29, 2019 •

edited

Loading