Skip to content
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

feat: introduces ability to suppress tracing via context #1344

Merged
merged 19 commits into from
Aug 25, 2020
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
207dfdc
feat: introduces ability to suppress tracing via context
michaelgoin Jul 21, 2020
f5d351c
feat: span processors prevent exporter span generation
michaelgoin Jul 23, 2020
98acb74
style: lint auto-fix
michaelgoin Jul 23, 2020
c2ac8b4
Merge branch 'master' into suppress-instrumentation
michaelgoin Jul 23, 2020
4d7e5ea
chore: suppressInstrumentation helper updates and cleanup
michaelgoin Jul 24, 2020
9a351ed
chore: resolve zalgo for InMemory/TracingTest span exporters
michaelgoin Jul 24, 2020
df7e85d
Merge branch 'master' into suppress-instrumentation
michaelgoin Jul 24, 2020
d588d4f
style: lint auto-fix
michaelgoin Jul 24, 2020
91d1add
Merge branch 'master' into suppress-instrumentation
michaelgoin Jul 24, 2020
9eb2d5e
Merge branch 'master' into suppress-instrumentation
vmarchaud Aug 12, 2020
d183bd8
Merge branch 'master' into suppress-instrumentation
vmarchaud Aug 15, 2020
4ff9591
Merge branch 'master' into suppress-instrumentation
michaelgoin Aug 20, 2020
6bb2eb6
refactor: resolves PR feedback
michaelgoin Aug 20, 2020
49f53e4
Merge branch 'suppress-instrumentation' of github.com:michaelgoin/ope…
michaelgoin Aug 20, 2020
74a4000
Merge branch 'master' into suppress-instrumentation
michaelgoin Aug 20, 2020
2866f51
refactor: splits setting of supress instrumentation into two function
michaelgoin Aug 25, 2020
6c81a98
Merge branch 'master' into suppress-instrumentation
michaelgoin Aug 25, 2020
cc0249b
refactor: simplifies isInstrumentationSuppressed boolean handling
michaelgoin Aug 25, 2020
f125085
style: lint:fix
michaelgoin Aug 25, 2020
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
38 changes: 38 additions & 0 deletions packages/opentelemetry-core/src/context/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ export const ACTIVE_SPAN_KEY = Context.createKey(
const EXTRACTED_SPAN_CONTEXT_KEY = Context.createKey(
'OpenTelemetry Context Key EXTRACTED_SPAN_CONTEXT'
);
/**
* Shared key for indicating if instrumentation should be suppressed beyond
* this current scope.
*/
export const SUPPRESS_INSTRUMENTATION_KEY = Context.createKey(
'OpenTelemetry Context Key SUPPRESS_INSTRUMENTATION'
);

/**
* Return the active span if one exists
Expand Down Expand Up @@ -84,3 +91,34 @@ export function getParentSpanContext(
): SpanContext | undefined {
return getActiveSpan(context)?.context() || getExtractedSpanContext(context);
}

/**
* Sets value on context to indicate that instrumentation should
* be suppressed beyond this current scope.
*
* @param context context to set the suppress instrumentation value on.
*/
export function suppressInstrumentation(context: Context): Context {
return context.setValue(SUPPRESS_INSTRUMENTATION_KEY, true);
}

/**
* Sets value on context to indicate that instrumentation should
* no-longer be suppressed beyond this current scope.
*
* @param context context to set the suppress instrumentation value on.
*/
export function unsuppressInstrumentation(context: Context): Context {
return context.setValue(SUPPRESS_INSTRUMENTATION_KEY, false);
}

/**
* Return current suppress instrumentation value for the given context,
* if it exists.
*
* @param context context check for the suppress instrumentation value.
*/
export function isInstrumentationSuppressed(context: Context): boolean {
const value = context.getValue(SUPPRESS_INSTRUMENTATION_KEY) as boolean;
return !!value;
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the extraneous cast and assignment, but this is minor.

Suggested change
const value = context.getValue(SUPPRESS_INSTRUMENTATION_KEY) as boolean;
return !!value;
return Boolean(context.getValue(SUPPRESS_INSTRUMENTATION_KEY));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was definitely giving that the side-eye while I was restructuring. Thanks for the extra push. :)

Copy link
Member

Choose a reason for hiding this comment

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

It's probably minor, but since this function is likely to be called a lot, I think the performance benefit alone of skipping the assignment is worth it since you don't really lose anything here.

}
91 changes: 91 additions & 0 deletions packages/opentelemetry-core/test/context/context.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as assert from 'assert';

import {
SUPPRESS_INSTRUMENTATION_KEY,
suppressInstrumentation,
unsuppressInstrumentation,
isInstrumentationSuppressed,
} from '../../src/context/context';
import { Context } from '@opentelemetry/api';

describe('Context Helpers', () => {
describe('suppressInstrumentation', () => {
it('should set suppress to true', () => {
const expectedValue = true;
const context = suppressInstrumentation(Context.ROOT_CONTEXT);

const value = context.getValue(SUPPRESS_INSTRUMENTATION_KEY);
const boolValue = value as boolean;

assert.equal(boolValue, expectedValue);
});
});

describe('unsuppressInstrumentation', () => {
it('should set suppress to false', () => {
const expectedValue = false;
const context = unsuppressInstrumentation(Context.ROOT_CONTEXT);

const value = context.getValue(SUPPRESS_INSTRUMENTATION_KEY);
const boolValue = value as boolean;

assert.equal(boolValue, expectedValue);
});
});

describe('isInstrumentationSuppressed', () => {
it('should get value as bool', () => {
const expectedValue = true;
const context = Context.ROOT_CONTEXT.setValue(
SUPPRESS_INSTRUMENTATION_KEY,
expectedValue
);

const value = isInstrumentationSuppressed(context);

assert.equal(value, expectedValue);
});

describe('when suppress instrumentation set to null', () => {
const context = Context.ROOT_CONTEXT.setValue(
SUPPRESS_INSTRUMENTATION_KEY,
null
);

it('should return false', () => {
const value = isInstrumentationSuppressed(context);

assert.equal(value, false);
});
});

describe('when suppress instrumentation set to undefined', () => {
const context = Context.ROOT_CONTEXT.setValue(
SUPPRESS_INSTRUMENTATION_KEY,
undefined
);

it('should return false', () => {
const value = isInstrumentationSuppressed(context);

assert.equal(value, false);
});
});
});
});
7 changes: 7 additions & 0 deletions packages/opentelemetry-tracing/src/Tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
IdGenerator,
RandomIdGenerator,
setActiveSpan,
isInstrumentationSuppressed,
} from '@opentelemetry/core';
import { Resource } from '@opentelemetry/resources';
import { BasicTracerProvider } from './BasicTracerProvider';
Expand Down Expand Up @@ -69,6 +70,11 @@ export class Tracer implements api.Tracer {
options: api.SpanOptions = {},
context = api.context.active()
): api.Span {
if (isInstrumentationSuppressed(context)) {
this.logger.debug('Instrumentation suppressed, returning Noop Span');
return api.NOOP_SPAN;
}

const parentContext = getParent(options, context);
const spanId = this._idGenerator.generateSpanId();
let traceId;
Expand All @@ -81,6 +87,7 @@ export class Tracer implements api.Tracer {
traceId = parentContext.traceId;
traceState = parentContext.traceState;
}

const spanKind = options.kind ?? api.SpanKind.INTERNAL;
const links = options.links ?? [];
const attributes = options.attributes ?? {};
Expand Down
10 changes: 8 additions & 2 deletions packages/opentelemetry-tracing/src/export/BatchSpanProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
* limitations under the License.
*/

import { unrefTimer } from '@opentelemetry/core';
import { context } from '@opentelemetry/api';
import { unrefTimer, suppressInstrumentation } from '@opentelemetry/core';
import { SpanProcessor } from '../SpanProcessor';
import { BufferConfig } from '../types';
import { ReadableSpan } from './ReadableSpan';
Expand Down Expand Up @@ -88,7 +89,12 @@ export class BatchSpanProcessor implements SpanProcessor {
setTimeout(cb, 0);
return;
}
this._exporter.export(this._finishedSpans, cb);

// prevent downstream exporter calls from generating spans
context.with(suppressInstrumentation(context.active()), () => {
this._exporter.export(this._finishedSpans, cb);
});

this._finishedSpans = [];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,25 @@ import { ExportResult } from '@opentelemetry/core';

/**
* This class can be used for testing purposes. It stores the exported spans
* in a list in memory that can be retrieve using the `getFinishedSpans()`
* in a list in memory that can be retrieved using the `getFinishedSpans()`
* method.
*/
export class InMemorySpanExporter implements SpanExporter {
private _finishedSpans: ReadableSpan[] = [];
private _stopped = false;
/**
* Indicates if the exporter has been "shutdown."
* When false, exported spans will not be stored in-memory.
*/
protected _stopped = false;
Copy link
Member

Choose a reason for hiding this comment

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

please add some doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@obecny what sort of doc are you looking for?

do you want a comment on why this is protected? that seems like something that would quickly get out of date with future usages, outside of the obvious fact it is now available to classes extending the InMemorySpanExporter.

or are you looking for something else?

Copy link
Member

Choose a reason for hiding this comment

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

If something is protected its meaning should be documented so that when someone subclasses this they know what the property represents. It could be argued it is obvious in this case, but it is good practice in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. i don't see that done for any other protected members (non-function) in the code base and somewhat debate the value here but am happy to go along with the standards y'all are looking for.

Copy link
Member

Choose a reason for hiding this comment

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

@michaelgoin I agree we haven't been great at being consistent about this, and in many cases the value is questionable anyways.


export(
spans: ReadableSpan[],
resultCallback: (result: ExportResult) => void
): void {
if (this._stopped) return resultCallback(ExportResult.FAILED_NOT_RETRYABLE);
this._finishedSpans.push(...spans);
return resultCallback(ExportResult.SUCCESS);

setTimeout(() => resultCallback(ExportResult.SUCCESS), 0);
}

shutdown(): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import { SpanProcessor } from '../SpanProcessor';
import { SpanExporter } from './SpanExporter';
import { ReadableSpan } from './ReadableSpan';
import { context } from '@opentelemetry/api';
import { suppressInstrumentation } from '@opentelemetry/core';

/**
* An implementation of the {@link SpanProcessor} that converts the {@link Span}
Expand All @@ -40,7 +42,11 @@ export class SimpleSpanProcessor implements SpanProcessor {
if (this._isShutdown) {
return;
}
this._exporter.export([span], () => {});

// prevent downstream exporter calls from generating spans
context.with(suppressInstrumentation(context.active()), () => {
this._exporter.export([span], () => {});
});
}

shutdown(cb: () => void = () => {}): void {
Expand Down
22 changes: 22 additions & 0 deletions packages/opentelemetry-tracing/test/Tracer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
NoopSpan,
Sampler,
SamplingDecision,
Context,
NOOP_SPAN,
TraceFlags,
} from '@opentelemetry/api';
import { BasicTracerProvider, Tracer, Span } from '../src';
Expand All @@ -27,6 +29,7 @@ import {
NoopLogger,
AlwaysOnSampler,
AlwaysOffSampler,
suppressInstrumentation,
} from '@opentelemetry/core';

describe('Tracer', () => {
Expand Down Expand Up @@ -115,6 +118,25 @@ describe('Tracer', () => {
assert.strictEqual(lib.version, '0.0.1');
});

describe('when suppressInstrumentation true', () => {
const context = suppressInstrumentation(Context.ROOT_CONTEXT);

it('should return cached no-op span ', done => {
const tracer = new Tracer(
{ name: 'default', version: '0.0.1' },
{ sampler: new TestSampler() },
tracerProvider
);

const span = tracer.startSpan('span3', undefined, context);

assert.equal(span, NOOP_SPAN);
span.end();

done();
});
});

if (typeof process !== 'undefined' && process.release.name === 'node') {
it('should sample a trace when OTEL_SAMPLING_PROBABILITY is invalid', () => {
process.env.OTEL_SAMPLING_PROBABILITY = 'invalid value';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import {
InMemorySpanExporter,
Span,
} from '../../src';
import { context } from '@opentelemetry/api';
import { TestTracingSpanExporter } from './TestTracingSpanExporter';
import { TestStackContextManager } from './TestStackContextManager';

function createSampledSpan(spanName: string): Span {
const tracer = new BasicTracerProvider({
Expand Down Expand Up @@ -226,5 +229,32 @@ describe('BatchSpanProcessor', () => {
});
});
});

describe('flushing spans with exporter triggering instrumentation', () => {
beforeEach(() => {
const contextManager = new TestStackContextManager().enable();
context.setGlobalContextManager(contextManager);
});

afterEach(() => {
context.disable();
});

it('should prevent instrumentation prior to export', done => {
const testTracingExporter = new TestTracingSpanExporter();
const processor = new BatchSpanProcessor(testTracingExporter);

const span = createSampledSpan('test');
processor.onStart(span);
processor.onEnd(span);

processor.forceFlush(() => {
const exporterCreatedSpans = testTracingExporter.getExporterCreatedSpans();
assert.equal(exporterCreatedSpans.length, 0);

done();
});
});
});
});
});
Loading