Support delayed sampling by yurishkuro · Pull Request #380 · jaegertracing/jaeger-client-node

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 <[email protected]>

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

    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

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

  _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

  }

+  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

  _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".

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

+  }
+
+  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

+      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

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

I think the API looks pretty usable

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

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

+        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

+
+  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

+    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 <[email protected]>

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

+        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

+
+  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 <[email protected]>

jaegertracing#380)

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

Support late sampling decisions

3f74c34

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

Extract countdown callback into util

683ca38

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

Add PrioritySampler

784c5f8

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

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 <[email protected]>

Clean up flwo errors

c4cafd1

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

Do not use array.fill()

706d57c

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

Add tests for remote parent

76be657

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

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 <[email protected]>

Upgrade probabilistic and rate limiting samplers to v2

f74eae1

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

Upgrade most samplers to V2

f2ee493

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

Fix flow error

6fe64c0

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

Cleanup some stuff

5f0f5e1

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

Test that per-operation-sampler still behaves as before

d077561

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

yurishkuro commented Sep 3, 2019

View reviewed changes

vprithvi reviewed Sep 3, 2019

View reviewed changes

yurishkuro commented Sep 3, 2019

View reviewed changes

Comment thread test/samplers/delayed_sampling.js Outdated

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

Address review comments

a1cb4ae

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

Simplify with inheritance

689652b

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

Move new samplers to /experimental/

714f55c

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

Support firehose in tag sampler

6bc8f20

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

Fix flow issue

cfe6768

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

Expose loggers

ae7dc12

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

Extend remote sampler in test

4cd7ce2

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

Increase code coverage

88436f7

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

Add more tests

79cdc3d

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

Fix flow error

352dbcc

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

Add tests

6241aae

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

yurishkuro commented Sep 4, 2019

View reviewed changes

Comment thread src/samplers/experimental/tag_equals_sampler.js Outdated

More tests

31cdc22

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

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 <[email protected]>

vprithvi approved these changes Sep 4, 2019

View reviewed changes

Use 'classicStrategy' property

465599c

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

vprithvi approved these changes Sep 4, 2019

View reviewed changes

Fix flow error

0454958

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

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)

Conversation

yurishkuro commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Short description of the changes

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

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!

Uh oh!

vprithvi commented Sep 3, 2019

Uh oh!

Uh oh!

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!

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!

2 participants

yurishkuro commented Aug 29, 2019 •

edited

Loading