Skip to content
This repository has been archived by the owner on Jul 10, 2023. It is now read-only.

Add custom sampler Trace Plugin to sample individual spans #50

Merged
merged 5 commits into from
Mar 19, 2019
Merged

Add custom sampler Trace Plugin to sample individual spans #50

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f62f909
Add custom sampler Trace Plugin to sample individual spans
Mar 14, 2019
3e1baf9
Wrap callbackWaitsForEmptyEventLoop setter and getter
Mar 14, 2019
a68315d
Change customSampler config to Sampler instead of function returning …
Mar 14, 2019
a44c6f9
Change log message when custom filtering is applied
Mar 14, 2019
187d3d3
add `runCustomSamplerOnEachSpan` config to explicitly run sampler on …
Mar 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"scripts": {
"precompile": "node create-build-info.js",
"compile": "tsc",
"build": "node create-build-info.js;webpack;",
"build": "node create-build-info.js; mv ./dist/BuildInfo.js ./src; webpack; rm ./src/BuildInfo.js",
"start": "npm-run-all --parallel watch:server watch:build",
"test-automatic-instrumentation": "node ./test/instrumentation/automated.instrumentation.js",
"test": "npm run compile; npm run test-automatic-instrumentation; docker-compose up -d; jest --coverage; docker-compose stop",
Expand Down
2 changes: 1 addition & 1 deletion src/BuildInfoLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const BuildInfo = require('../package.json');
const BuildInfo = require('./BuildInfo');
class BuildInfoLoader {
static getAgentVersion(): string {
return BuildInfo.version;
Expand Down
17 changes: 13 additions & 4 deletions src/ThundraWrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,16 @@ class ThundraWrapper {
},
};

const me = this;
this.wrappedContext = Object.assign({
set callbackWaitsForEmptyEventLoop(value) {
me.originalContext.callbackWaitsForEmptyEventLoop = value;
},
get callbackWaitsForEmptyEventLoop() {
return me.originalContext.callbackWaitsForEmptyEventLoop;
},
}, this.wrappedContext);

this.timeout = this.setupTimeoutHandler(this);
}

Expand All @@ -97,6 +107,7 @@ class ThundraWrapper {
this.executeHook('before-invocation', beforeInvocationData, false)
.then(() => {
this.pluginContext.requestCount += 1;
this.resetTime();
this.pluginContext.invocationStartTimestamp = Date.now();

try {
Expand Down Expand Up @@ -138,8 +149,6 @@ class ThundraWrapper {
}

async executeAfteInvocationAndReport(afterInvocationData: any) {
this.pluginContext.invocationFinishTimestamp = Date.now();

await this.executeHook('after-invocation', afterInvocationData, true);
this.resetTime();

Expand Down Expand Up @@ -171,7 +180,7 @@ class ThundraWrapper {

if (this.config.sampleTimedOutInvocations) {
if (error instanceof TimeoutError) {
this.executeAfteInvocationAndReport(afterInvocationData);
await this.executeAfteInvocationAndReport(afterInvocationData);
} else {
this.plugins.map((plugin: any) => {
if (plugin.destroy && typeof (plugin.destroy) === 'function') {
Expand All @@ -180,7 +189,7 @@ class ThundraWrapper {
});
}
} else {
this.executeAfteInvocationAndReport(afterInvocationData);
await this.executeAfteInvocationAndReport(afterInvocationData);
}

if (this.timeout) {
Expand Down
9 changes: 9 additions & 0 deletions src/plugins/Trace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,18 @@ export class Trace {

const spanList = this.tracer.getRecorder().getSpanList();
const sampled = (this.config && this.config.samplerConfig) ? this.config.samplerConfig.isSampled(this.rootSpan) : true;
const customSampler = (this.config && this.config.samplerConfig && this.config.samplerConfig.customSampler) ?
this.config.samplerConfig.customSampler : false;

if (sampled) {
for (const span of spanList) {
if (span) {
if (customSampler && !customSampler.isSampled(span)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Individual span should not be sampled but root span. Because it might break parent-child relation between spans and leads to problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if root span is not sampled, all of the others spans should not be sampled and ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User should explicitly set runCustomSamplerOnEachSpan to apply custom sampler on each span and the responsibility will be on them. When runCustomSamplerOnEachSpan is not set
or set the false, custom sampler is applied to only root span and root span is not sampled no chlid span is sampled.

The configuration will look like this.

const config = {
    traceConfig: {
        samplerConfig: {
            runCustomSamplerOnEachSpan: true,
            customSampler:{
                isSampled(span) {
                    // custom sample logic is applied here
                    return true;
                }
            }
        }
    }
}

ThundraLogger.getInstance().debug(
`Filtering span with name ${span.getOperationName()} due to custom sampling configration`);
continue;
}

const spanData = this.buildSpanData(span, this.pluginContext);
const spanReportData = Utils.generateReport(spanData, this.apiKey);
this.report(spanReportData);
Expand Down
10 changes: 2 additions & 8 deletions src/plugins/config/TraceSamplerConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class TraceSamplerConfig {
errorAwareSamplerConfig: ErrorAwareSamplerConfig;
durationAwareSampler: DurationAwareSampler;
errorAwareSampler: ErrorAwareSampler;
customSampler: () => Sampler<ThundraSpan>;
customSampler: Sampler<ThundraSpan>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where we use customSampler for sampling?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok got it, never mind


constructor(options: any) {
options = options ? options : {};
Expand All @@ -30,7 +30,7 @@ class TraceSamplerConfig {

isSampled(span: ThundraSpan): boolean {
if (!this.durationAwareSampler &&
!this.errorAwareSampler && !this.customSampler) {
!this.errorAwareSampler) {
return true;
}

Expand All @@ -44,12 +44,6 @@ class TraceSamplerConfig {
isSampled = isSampled || this.errorAwareSampler.isSampled(span);
}

if (this.customSampler) {
if (typeof this.customSampler === 'function' && this.customSampler().isSampled) {
isSampled = isSampled || this.customSampler().isSampled();
}
}

return isSampled;
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/plugins/invocation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ describe('Invocation', () => {
expect(invocation.apiKey).toBe(pluginContext.apiKey);
expect(invocation.invocationData.id).toBeTruthy();
expect(invocation.invocationData.type).toEqual('Invocation');
expect(semver.inc(invocation.invocationData.agentVersion, 'patch')).toEqual(buildInfo.version);
expect(invocation.invocationData.agentVersion).toEqual(buildInfo.version);
expect(invocation.invocationData.dataModelVersion).toEqual(DATA_MODEL_VERSION);
expect(invocation.invocationData.applicationId).toEqual(pluginContext.applicationId);
expect(invocation.invocationData.applicationDomainName).toEqual(LAMBDA_APPLICATION_DOMAIN_NAME);
Expand Down