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(http-plugin): add options to disable new spans if no parent #931 #948

Merged
merged 7 commits into from
May 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 2 additions & 0 deletions packages/opentelemetry-plugin-http/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ Http plugin has few options available to choose from. You can set the following:
| [`ignoreIncomingPaths`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `IgnoreMatcher[]` | Http plugin will not trace all incoming requests that match paths |
| [`ignoreOutgoingUrls`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `IgnoreMatcher[]` | Http plugin will not trace all outgoing requests that match urls |
| [`serverName`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `string` | The primary server name of the matched virtual host. |
| `requireParentforOutgoingSpans` | Boolean | Require that is a parent span to create new span for outgoing requests. |
| `requireParentforIncomingSpans` | Boolean | Require that is a parent span to create new span for incoming requests. |

## Useful links
- For more information on OpenTelemetry, visit: <https://opentelemetry.io/>
Expand Down
38 changes: 33 additions & 5 deletions packages/opentelemetry-plugin-http/src/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@ import {
SpanKind,
SpanOptions,
Status,
SpanContext,
TraceFlags,
} from '@opentelemetry/api';
import { BasePlugin } from '@opentelemetry/core';
import {
BasePlugin,
NoRecordingSpan,
getExtractedSpanContext,
} from '@opentelemetry/core';
import {
ClientRequest,
IncomingMessage,
Expand Down Expand Up @@ -57,6 +63,12 @@ export class HttpPlugin extends BasePlugin<Http> {
/** keep track on spans not ended */
private readonly _spanNotEnded: WeakSet<Span>;

private readonly _emptySpanContext: SpanContext = {
traceId: '',
spanId: '',
traceFlags: TraceFlags.NONE,
};

constructor(readonly moduleName: string, readonly version: string) {
super(`@opentelemetry/plugin-${moduleName}`, VERSION);
// For now component is equal to moduleName but it can change in the future.
Expand Down Expand Up @@ -396,7 +408,6 @@ export class HttpPlugin extends BasePlugin<Http> {
const spanOptions: SpanOptions = {
kind: SpanKind.CLIENT,
};

const span = plugin._startHttpSpan(operationName, spanOptions);

return plugin._tracer.withSpan(span, () => {
Expand All @@ -417,9 +428,26 @@ export class HttpPlugin extends BasePlugin<Http> {
}

private _startHttpSpan(name: string, options: SpanOptions) {
const span = this._tracer
.startSpan(name, options)
.setAttribute(AttributeNames.COMPONENT, this.component);
/*
* If a parent is required but not present, we use a `NoRecordingSpan` to still
* propagate context without recording it.
*/
const requireParent =
options.kind === SpanKind.CLIENT
? this._config.requireParentforOutgoingSpans
: this._config.requireParentforIncomingSpans;
let span: Span;
if (requireParent === true && this._tracer.getCurrentSpan() === undefined) {
const spanContext =
getExtractedSpanContext(context.active()) ?? plugin._emptySpanContext;
// TODO: Refactor this when a solution is found in
// https://github.com/open-telemetry/opentelemetry-specification/issues/530
span = new NoRecordingSpan(spanContext);
} else {
span = this._tracer
.startSpan(name, options)
.setAttribute(AttributeNames.COMPONENT, this.component);
}
this._spanNotEnded.add(span);
return span;
}
Expand Down
4 changes: 4 additions & 0 deletions packages/opentelemetry-plugin-http/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ export interface HttpPluginConfig extends PluginConfig {
applyCustomAttributesOnSpan?: HttpCustomAttributeFunction;
/** The primary server name of the matched virtual host. */
serverName?: string;
/** Require parent to create span for outgoing requests */
requireParentforOutgoingSpans?: boolean;
/** Require parent to create span for incoming requests */
requireParentforIncomingSpans?: boolean;
}

export interface Err extends Error {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -696,5 +696,125 @@ describe('HttpPlugin', () => {
req.end();
});
});

describe('with require parent span', () => {
beforeEach(done => {
memoryExporter.reset();
plugin.enable(http, provider, provider.logger, {});
server = http.createServer((request, response) => {
response.end('Test Server Response');
});
server.listen(serverPort, done);
});

afterEach(() => {
server.close();
plugin.disable();
});

it(`should not trace without parent with options enabled (both client & server)`, async () => {
plugin.disable();
const config: HttpPluginConfig = {
requireParentforIncomingSpans: true,
requireParentforOutgoingSpans: true,
};
plugin.enable(http, provider, provider.logger, config);
const testPath = `/test/test`;
await httpRequest.get(
`${protocol}://${hostname}:${serverPort}${testPath}`
);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans.length, 0);
});

it(`should not trace without parent with options enabled (client only)`, async () => {
plugin.disable();
const config: HttpPluginConfig = {
requireParentforOutgoingSpans: true,
};
plugin.enable(http, provider, provider.logger, config);
const testPath = `/test/test`;
const result = await httpRequest.get(
`${protocol}://${hostname}:${serverPort}${testPath}`
);
assert(
result.reqHeaders[DummyPropagation.TRACE_CONTEXT_KEY] !== undefined
);
assert(
result.reqHeaders[DummyPropagation.SPAN_CONTEXT_KEY] !== undefined
);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans.length, 1);
assert.strictEqual(
spans.every(span => span.kind === SpanKind.SERVER),
true
);
});

it(`should not trace without parent with options enabled (server only)`, async () => {
plugin.disable();
const config: HttpPluginConfig = {
requireParentforIncomingSpans: true,
};
plugin.enable(http, provider, provider.logger, config);
const testPath = `/test/test`;
const result = await httpRequest.get(
`${protocol}://${hostname}:${serverPort}${testPath}`
);
assert(
result.reqHeaders[DummyPropagation.TRACE_CONTEXT_KEY] !== undefined
);
assert(
result.reqHeaders[DummyPropagation.SPAN_CONTEXT_KEY] !== undefined
);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans.length, 1);
assert.strictEqual(
spans.every(span => span.kind === SpanKind.CLIENT),
true
);
});

it(`should trace with parent with both requireParent options enabled`, done => {
plugin.disable();
const config: HttpPluginConfig = {
requireParentforIncomingSpans: true,
requireParentforOutgoingSpans: true,
};
plugin.enable(http, provider, provider.logger, config);
const testPath = `/test/test`;
const tracer = provider.getTracer('default');
const span = tracer.startSpan('parentSpan', {
kind: SpanKind.INTERNAL,
});
tracer.withSpan(span, () => {
httpRequest
.get(`${protocol}://${hostname}:${serverPort}${testPath}`)
.then(result => {
span.end();
assert(
result.reqHeaders[DummyPropagation.TRACE_CONTEXT_KEY] !==
undefined
);
assert(
result.reqHeaders[DummyPropagation.SPAN_CONTEXT_KEY] !==
undefined
);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans.length, 2);
assert.strictEqual(
spans.filter(span => span.kind === SpanKind.CLIENT).length,
1
);
assert.strictEqual(
spans.filter(span => span.kind === SpanKind.INTERNAL).length,
1
);
return done();
})
.catch(done);
});
});
});
});
});