feat: Added partial granularity samplers and assign transactions with isPartialTrace when partial granularity sampling decisions have been made
#3544
Conversation
… `isPartialTrace` when partial granularity sampling decisions have been made
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3544 +/- ##
==========================================
- Coverage 97.77% 97.66% -0.11%
==========================================
Files 427 427
Lines 56099 56244 +145
Branches 1 1
==========================================
+ Hits 54853 54933 +80
- Misses 1246 1311 +65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I may have missed it in test/unit/samplers/index.test.js, but I do not see an integration test testing the following scenario that we've commonly brought up (I see the unit test that affirms the "effective ratio" addition):
If there are 100 traces, agent.samplers.root.ratio = 0.4 and agent.samplers.partialRoot.ratio = 0.2, will there be ~40 full traces and ~20 partial traces?
|
|
||
| class Samplers { | ||
| constructor(agent) { | ||
| this.fullEnabled = agent.config.distributed_tracing.sampler.full_granularity.enabled |
There was a problem hiding this comment.
Do we have to check for distributed_tracing.enabled here?
There was a problem hiding this comment.
I don't want to change too much here. But previously we always assigned priority and sampled. Then before we generate span events we check if DT is enabled here
Do we need a test? As you mentioned I have tests that assert the ratio is assigned correctly. We also have a full suite of tests for one ratio sampler |
Actually let me clean up the ratio unit tests, they are misleading. and I'll add a test for this |
amychisholm03
left a comment
There was a problem hiding this comment.
Ideally, we'd have tests to test other full/partial sampler combinations, but I think the full ratio and partial ratio test is good for now
|
I'm going to merge this but I have a question out about how to handle assigning priority when DT is disabled |
jsumners-nr
left a comment
There was a problem hiding this comment.
It's all looking reasonable to me. I have a few comments, but none that would have been blockers on this.
| const sampler = isSampled ? this.remoteParentSampled : this.remoteParentNotSampled | ||
| const partialSampler = isSampled ? this.partialRemoteParentSampled : this.partialRemoteParentNotSampled | ||
| if (this.fullEnabled) { | ||
| if (sampler?.toString() === 'AdaptiveSampler') { |
There was a problem hiding this comment.
I see that you've implemented a literal toString method on these objects:
node-newrelic/lib/samplers/adaptive-sampler.js
Lines 42 to 44 in 1535a82
These implementations are a bit odd because they seem to be attempting to provide "type" inference support. Whereas a typical toString override would actually return some sort of string representation of the object. Consider the following:
'use strict'
class Foo {
toString() {
return 'Foo'
}
}
const foo = new Foo()
console.log(
Object.prototype.toString.call(foo)
)
console.log(
foo.toString()
)
class Bar {
get [Symbol.toStringTag]() { return 'Bar' }
toString() { return 'Bar' }
}
const bar = new Bar()
console.log(
Object.prototype.toString.call(bar)
)
console.log(
bar.toString()
)The output of that script is:
[object Object]
Foo
[object Bar]
Bar
Which is to say, the overrides in these methods are contrary to what one would expect if they are familiar with the canonical way of determining the "type" of an object (using Object.prototype.toString).
There was a problem hiding this comment.
I didn't implement this. it was in the previous iterations. I agree but didn't change it in here
| 'use strict' | ||
|
|
||
| const test = require('node:test') | ||
| const assert = require('node:assert') | ||
|
|
||
| const Config = require('#agentlib/config/index.js') | ||
| const Samplers = require('#agentlib/samplers/index.js') | ||
| const AdaptiveSampler = require('#agentlib/samplers/adaptive-sampler.js') | ||
| const AlwaysOnSampler = require('#agentlib/samplers/always-on-sampler.js') | ||
| const AlwaysOffSampler = require('#agentlib/samplers/always-off-sampler.js') | ||
| const TraceIdRatioBasedSampler = require('#agentlib/samplers/ratio-based-sampler.js') | ||
|
|
||
| function beforeEach(ctx) { |
There was a problem hiding this comment.
I am so happy to see this clean organization of imports.
Description
This PR encapsulates all the sampling decision executions into a
Samplersclass. It instantiates up to 7 samplers and applies the correct one depending on your configuration and where a sampling decision is made. For a majority of the sampling decisions, the will be made withsamplers.applySamplingDecisionwhich run both full and partial granularity root samplers depending on your configuration. In the case of accepting a DT trace from an upstream parent, it will run both full and partial granularity remoteParentSampled or remoteParentNotSampled depending on the traceparent or tracestate sampled flag. Lastly, the case of accepting a DT trace via thenewrelicheader, it will run both fulll and partial granularity remoteParentSampled and remoteParentNotSampled from the newrelic sampled flag.A few nuances to call out:
transaction.isPartialTrace. This will tell our span events to run the relevant mode of partial granularity(which will be done in Add support forsampler.partial_granularity, starting with"reduced"type #3455, Add support for"essential"partial granularity #3456, Add support for"compact"partial granularity #3454)Related Issues
Closes #3536