-
Notifications
You must be signed in to change notification settings - Fork 12
feat(trace): use dynamic composite ratio sampler #1070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
2e0f892
97f7e25
46b6c3d
bf92210
61dbf35
32dbc64
d231938
68f4bbd
d8a346f
ee65525
f1211c1
aff1225
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -420,6 +420,46 @@ const REMOTE_CONFIG_HANDLERS = [ | |||||||||||||||||||||||||||||||||||||||
return null; | ||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||
keys: ['sampling_rate'], | ||||||||||||||||||||||||||||||||||||||||
setter: (config, sdkInfo) => { | ||||||||||||||||||||||||||||||||||||||||
if (!sdkInfo.sampler) { | ||||||||||||||||||||||||||||||||||||||||
log.info( | ||||||||||||||||||||||||||||||||||||||||
`central-config: ignoring "sampling_rate" because non-default sampler in use` | ||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||
return null; | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
const rawRate = config['sampling_rate']; | ||||||||||||||||||||||||||||||||||||||||
let valRate; | ||||||||||||||||||||||||||||||||||||||||
switch (typeof rawRate) { | ||||||||||||||||||||||||||||||||||||||||
case 'undefined': | ||||||||||||||||||||||||||||||||||||||||
valRate = initialConfig.sampling_rate; | ||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||
case 'number': | ||||||||||||||||||||||||||||||||||||||||
valRate = rawRate; | ||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||
case 'string': | ||||||||||||||||||||||||||||||||||||||||
valRate = Number(rawRate); | ||||||||||||||||||||||||||||||||||||||||
if (isNaN(valRate)) { | ||||||||||||||||||||||||||||||||||||||||
return `unknown 'sampling_rate' value: "${rawRate}"`; | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||||||||||||||
return `unknown 'sampling_rate' value type: ${typeof rawRate} (${rawRate})`; | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
trentm marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||
if (valRate < 0 || valRate > 1) { | ||||||||||||||||||||||||||||||||||||||||
return `'sampling_rate' value must be between 0 and 1: ${valRate}`; | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
sdkInfo.sampler.setRatio(valRate); | ||||||||||||||||||||||||||||||||||||||||
log.info(`central-config: set "sampling_rate" to "${valRate}"`); | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
return null; | ||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||
]; | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||
|
@@ -598,8 +638,10 @@ function setupCentralConfig(sdkInfo) { | |||||||||||||||||||||||||||||||||||||||
CC_LOGGING_LEVEL_FROM_LUGGITE_LEVEL[ | ||||||||||||||||||||||||||||||||||||||||
luggite.nameFromLevel[log.level()] ?? DEFAULT_LOG_LEVEL | ||||||||||||||||||||||||||||||||||||||||
]; | ||||||||||||||||||||||||||||||||||||||||
initialConfig.sampling_rate = sdkInfo.samplingRate; | ||||||||||||||||||||||||||||||||||||||||
initialConfig.send_traces = !sdkInfo.contextPropagationOnly; | ||||||||||||||||||||||||||||||||||||||||
log.debug({initialConfig}, 'initial central config values'); | ||||||||||||||||||||||||||||||||||||||||
lastAppliedConfig = {...initialConfig}; | ||||||||||||||||||||||||||||||||||||||||
|
if (process.env.ELASTIC_OTEL_OPAMP_ENDPOINT) { | |
await setElasticConfig({logging_level: 'debug'}); | |
await barrierOpAMPClientDiagEvents(3, [DIAG_CH_SEND_SUCCESS]); | |
} | |
// 3. Now the `diag.debug` should result in an emitted record. | |
diag.verbose('verbose2'); | |
diag.debug('debug2'); | |
diag.info('info2'); | |
diag.warn('warn2'); | |
diag.error('error2'); | |
// 4. Set central config to empty config, to simulate an Agent Configuration | |
// in Kibana being removed. We expect the EDOT Node.js SDK to reset back | |
// to the default (info) log level. | |
if (process.env.ELASTIC_OTEL_OPAMP_ENDPOINT) { | |
await setElasticConfig({}); | |
await barrierOpAMPClientDiagEvents(3, [DIAG_CH_SEND_SUCCESS]); | |
} |
Personally I'm fine with not having an automated test for the undefined
case here: complexity/code-size of the tests is a trade-off. Or, I wouldn't object to a test case doing live-updating of the central config (as in the logging_level
example).
For consideration, I'll push a change that removes this and drops the test case for undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consideration, I'll push a change that removes this and drops the test case for
undefined
.
commit ee65525
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
const { | ||
createCompositeSampler, | ||
createComposableParentThresholdSampler, | ||
createComposableTraceIDRatioBasedSampler, | ||
} = require('@opentelemetry/sampler-composite'); | ||
|
||
/** | ||
* @typedef {import('@opentelemetry/api').Attributes} Attributes | ||
* @typedef {import('@opentelemetry/api').Context} Context | ||
* @typedef {import('@opentelemetry/api').Link} Link | ||
* @typedef {import('@opentelemetry/api').SpanKind} SpanKind | ||
* @typedef {import('@opentelemetry/sdk-trace-base').Sampler} Sampler | ||
* @typedef {import('@opentelemetry/sdk-trace-base').SamplingResult} SamplingResult | ||
*/ | ||
|
||
/** | ||
* EDOT default sampler, a parent-based ratio sampler which can have its ratio updated dynamically. | ||
* | ||
* @implements {Sampler} | ||
*/ | ||
class DefaultSampler { | ||
#delegate; | ||
|
||
constructor(ratio = 1.0) { | ||
this.#delegate = newSampler(ratio); | ||
} | ||
|
||
/** | ||
* @param {Context} context | ||
* @param {string} traceId | ||
* @param {string} spanName | ||
* @param {SpanKind} spanKind | ||
* @param {Attributes} attributes | ||
* @param {Link[]} links | ||
* @returns {SamplingResult} | ||
*/ | ||
shouldSample(context, traceId, spanName, spanKind, attributes, links) { | ||
return this.#delegate.shouldSample( | ||
context, | ||
traceId, | ||
spanName, | ||
spanKind, | ||
attributes, | ||
links | ||
); | ||
} | ||
|
||
/** | ||
* @param {number} ratio | ||
*/ | ||
setRatio(ratio) { | ||
this.#delegate = newSampler(ratio); | ||
} | ||
|
||
toString() { | ||
return this.#delegate.toString(); | ||
} | ||
} | ||
|
||
/** | ||
* @param {number} ratio A number between 0 and 1 representing the sampling ratio. | ||
*/ | ||
function newSampler(ratio) { | ||
return createCompositeSampler( | ||
createComposableParentThresholdSampler( | ||
createComposableTraceIDRatioBasedSampler(ratio) | ||
) | ||
); | ||
} | ||
|
||
/** | ||
* Creates a default EDOT sampler, which is a parent-based ratio sampler that can have | ||
* its ratio updated dynamically by central config. | ||
* | ||
* @param {number} ratio | ||
* @returns {Sampler} A ratio sampler which can have its ratio updated dynamically. | ||
*/ | ||
function createDefaultSampler(ratio) { | ||
trentm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return new DefaultSampler(ratio); | ||
} | ||
|
||
module.exports = { | ||
createDefaultSampler, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
|
||
const { | ||
getBooleanFromEnv, | ||
getNumberFromEnv, | ||
getStringFromEnv, | ||
getStringListFromEnv, | ||
} = require('@opentelemetry/core'); | ||
|
@@ -45,10 +46,12 @@ | |
setupDynConfExporters, | ||
dynConfSpanExporters, | ||
} = require('./dynconf'); | ||
const {createDefaultSampler} = require('./sampler'); | ||
const DISTRO_VERSION = require('../package.json').version; | ||
|
||
/** | ||
* @typedef {import('@opentelemetry/sdk-node').NodeSDKConfiguration} NodeSDKConfiguration | ||
* @typedef {import('@opentelemetry/sdk-trace-base').Sampler} Sampler | ||
*/ | ||
|
||
/** | ||
|
@@ -66,7 +69,7 @@ | |
try { | ||
await shutdownFn(); | ||
} catch (err) { | ||
console.warn('warning: error shutting down OTel SDK', err); | ||
} | ||
process.exit(128 + os.constants.signals.SIGTERM); | ||
}); | ||
|
@@ -76,7 +79,7 @@ | |
try { | ||
await shutdownFn(); | ||
} catch (err) { | ||
console.warn('warning: error shutting down OTel SDK', err); | ||
} | ||
}); | ||
} | ||
|
@@ -241,6 +244,26 @@ | |
|
||
const config = {...defaultConfig, ...cfg}; | ||
|
||
/** @type {Sampler} */ | ||
let sampler = undefined; | ||
let samplingRate = 1.0; | ||
if (!config.sampler && !getStringFromEnv('OTEL_TRACES_SAMPLER')) { | ||
// If the user has not set a sampler via config or env var, use our default sampler. | ||
// First get as string to differentiate between missing and invalid. | ||
if (getStringFromEnv('OTEL_TRACES_SAMPLER_ARG')) { | ||
const samplingRateArg = getNumberFromEnv('OTEL_TRACES_SAMPLER_ARG'); | ||
if (samplingRateArg === undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should also validate the
|
||
log.warn( | ||
`Invalid OTEL_TRACES_SAMPLER_ARG value: ${process.env.OTEL_TRACES_SAMPLER_ARG}. Using default sampling rate of ${samplingRate}` | ||
); | ||
} else { | ||
samplingRate = samplingRateArg; | ||
} | ||
} | ||
sampler = createDefaultSampler(samplingRate); | ||
config.sampler = sampler; | ||
} | ||
|
||
// Some tricks to get a handle on noop signal providers, to be used for | ||
// dynamic configuration. | ||
const tracerProviderProxy = new api.ProxyTracerProvider(); | ||
|
@@ -306,6 +329,8 @@ | |
noopTracerProvider, | ||
// @ts-ignore: Ignore access of private _tracerProvider for now. (TODO) | ||
sdkTracerProvider: sdk._tracerProvider, | ||
sampler, | ||
samplingRate, | ||
contextPropagationOnly, | ||
}); | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
const {test} = require('tape'); | ||
const {runTestFixtures} = require('./testutils'); | ||
|
||
/** @type {import('./testutils').TestFixture[]} */ | ||
const testFixtures = [ | ||
{ | ||
name: 'OTEL_TRACES_SAMPLER unset (default sampling of 100%)', | ||
args: ['./fixtures/use-http-get.js'], | ||
cwd: __dirname, | ||
env: { | ||
NODE_OPTIONS: '--import=@elastic/opentelemetry-node', | ||
}, | ||
checkTelemetry: (t, col) => { | ||
t.equal(col.sortedSpans.length, 1); | ||
}, | ||
}, | ||
{ | ||
name: 'OTEL_TRACES_SAMPLER unset, OTEL_TRACES_SAMPLER_ARG=0 (no sampling)', | ||
args: ['./fixtures/use-http-get.js'], | ||
cwd: __dirname, | ||
env: { | ||
NODE_OPTIONS: '--import=@elastic/opentelemetry-node', | ||
OTEL_TRACES_SAMPLER_ARG: '0', | ||
}, | ||
checkTelemetry: (t, col) => { | ||
t.equal(col.sortedSpans.length, 0); | ||
}, | ||
}, | ||
{ | ||
name: 'OTEL_TRACES_SAMPLER=always_off, (no sampling)', | ||
args: ['./fixtures/use-http-get.js'], | ||
cwd: __dirname, | ||
env: { | ||
NODE_OPTIONS: '--import=@elastic/opentelemetry-node', | ||
OTEL_TRACES_SAMPLER: 'always_off', | ||
}, | ||
checkTelemetry: (t, col) => { | ||
t.equal(col.sortedSpans.length, 0); | ||
}, | ||
}, | ||
]; | ||
|
||
// ----- main line ----- | ||
|
||
test('OTEL_TRACES_SAMPLER', (suite) => { | ||
runTestFixtures(suite, testFixtures); | ||
suite.end(); | ||
}); |
Uh oh!
There was an error while loading. Please reload this page.