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.

Sign up for GitHub

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

Jump to bottom

fix: add type checking in propagators #904

Merged
merged 4 commits into from
Mar 27, 2020
Merged

fix: add type checking in propagators #904

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8663f9
fix: add type checking in propagators
dyladan Mar 26, 2020
ffa02bb
chore: lint
dyladan Mar 26, 2020
c3edb73
test: add test for bad getter
dyladan Mar 26, 2020
24f540a
Merge branch 'master' into grcp-match-undefined
dyladan Mar 27, 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
4 changes: 3 additions & 1 deletion packages/opentelemetry-core/src/context/propagation/B3Propagator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ export class B3Propagator implements HttpTextPropagator {
const traceIdHeader = getter(carrier, X_B3_TRACE_ID);
const spanIdHeader = getter(carrier, X_B3_SPAN_ID);
const sampledHeader = getter(carrier, X_B3_SAMPLED);
if (!traceIdHeader || !spanIdHeader) return context;
const traceId = Array.isArray(traceIdHeader)
Copy link
Member Author

Choose a reason for hiding this comment

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

The root of this issue was that Array.isArray turns our unknown into an any which made the type system not complain like we would have expected.

? traceIdHeader[0]
: traceIdHeader;
Expand All @@ -75,6 +74,9 @@ export class B3Propagator implements HttpTextPropagator {
? sampledHeader[0]
: sampledHeader;

if (typeof traceId !== 'string' || typeof spanId !== 'string')
return context;

if (isValidTraceId(traceId) && isValidSpanId(spanId)) {
return setExtractedSpanContext(context, {
traceId,
Expand Down
5 changes: 4 additions & 1 deletion packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export class HttpTraceContext implements HttpTextPropagator {
const traceParent = Array.isArray(traceParentHeader)
? traceParentHeader[0]
: traceParentHeader;
if (typeof traceParent !== 'string') return context;
const spanContext = parseTraceParent(traceParent);
if (!spanContext) return context;

Expand All @@ -96,7 +97,9 @@ export class HttpTraceContext implements HttpTextPropagator {
const state = Array.isArray(traceStateHeader)
? traceStateHeader.join(',')
: traceStateHeader;
spanContext.traceState = new TraceState(state as string);
spanContext.traceState = new TraceState(
typeof state === 'string' ? state : undefined
);
}
return setExtractedSpanContext(context, spanContext);
}
Expand Down
22 changes: 22 additions & 0 deletions packages/opentelemetry-core/test/context/B3Propagator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,5 +267,27 @@ describe('B3Propagator', () => {
assert.deepStrictEqual(extractedSpanContext, undefined, testCase);
});
});

it('should fail gracefully on bad responses from getter', () => {
const ctx1 = b3Propagator.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => 1 // not a number
);
const ctx2 = b3Propagator.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => [] // empty array
);
const ctx3 = b3Propagator.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => undefined // missing value
);

assert.ok(ctx1 === Context.ROOT_CONTEXT);
assert.ok(ctx2 === Context.ROOT_CONTEXT);
assert.ok(ctx3 === Context.ROOT_CONTEXT);
});
});
});
22 changes: 22 additions & 0 deletions packages/opentelemetry-core/test/context/HttpTraceContext.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,5 +212,27 @@ describe('HttpTraceContext', () => {
assert.deepStrictEqual(extractedSpanContext, undefined, testCase);
});
});

it('should fail gracefully on bad responses from getter', () => {
const ctx1 = httpTraceContext.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => 1 // not a number
);
const ctx2 = httpTraceContext.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => [] // empty array
);
const ctx3 = httpTraceContext.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => undefined // missing value
);

assert.ok(ctx1 === Context.ROOT_CONTEXT);
assert.ok(ctx2 === Context.ROOT_CONTEXT);
assert.ok(ctx3 === Context.ROOT_CONTEXT);
});
});
});
3 changes: 2 additions & 1 deletion packages/opentelemetry-propagator-jaeger/src/JaegerHttpTracePropagator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,12 @@ export class JaegerHttpTracePropagator implements HttpTextPropagator {

extract(context: Context, carrier: unknown, getter: GetterFunction): Context {
const uberTraceIdHeader = getter(carrier, this._jaegerTraceHeader);
if (!uberTraceIdHeader) return context;
const uberTraceId = Array.isArray(uberTraceIdHeader)
? uberTraceIdHeader[0]
: uberTraceIdHeader;

if (typeof uberTraceId !== 'string') return context;

const spanContext = deserializeSpanContext(uberTraceId);
if (!spanContext) return context;

Expand Down
22 changes: 22 additions & 0 deletions packages/opentelemetry-propagator-jaeger/test/JaegerHttpTracePropagator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,26 @@ describe('JaegerHttpTracePropagator', () => {
);
});
});

it('should fail gracefully on bad responses from getter', () => {
const ctx1 = jaegerHttpTracePropagator.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => 1 // not a number
);
const ctx2 = jaegerHttpTracePropagator.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => [] // empty array
);
const ctx3 = jaegerHttpTracePropagator.extract(
Context.ROOT_CONTEXT,
carrier,
(c, k) => undefined // missing value
);

assert.ok(ctx1 === Context.ROOT_CONTEXT);
assert.ok(ctx2 === Context.ROOT_CONTEXT);
assert.ok(ctx3 === Context.ROOT_CONTEXT);
});
});