Skip to content

Commit 56bb988

Browse files
isaacsisaacs
committed
review fixes
1 parent 7044c34 commit 56bb988

5 files changed

Lines changed: 172 additions & 144 deletions

File tree

‎packages/core/src/integrations/express/patch-layer.ts‎

Lines changed: 153 additions & 131 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,11 @@ export type ExpressPatchLayerOptions = Pick<
3030
'onRouteResolved' | 'ignoreLayers' | 'ignoreLayersType'
3131
>;
3232

33-
export function patchLayer(options: ExpressPatchLayerOptions, layer?: ExpressLayer, layerPath?: string): void {
34-
if (!layer) return;
33+
export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: ExpressLayer, layerPath?: string): void {
34+
if (!maybeLayer) {
35+
return;
36+
}
37+
const layer = maybeLayer;
3538

3639
// avoid patching multiple times the same layer
3740
if (layer[kLayerPatched] === true) return;
@@ -43,146 +46,165 @@ export function patchLayer(options: ExpressPatchLayerOptions, layer?: ExpressLay
4346
return;
4447
}
4548

46-
Object.defineProperty(layer, 'handle', {
47-
enumerable: true,
48-
configurable: true,
49-
value: function layerHandlePatched(
50-
this: ExpressLayer,
51-
req: ExpressRequest,
52-
res: ExpressResponse,
53-
//oxlint-disable-next-line no-explicit-any
54-
...otherArgs: any[]
55-
) {
56-
// Only create spans when there's an active parent span
57-
// Without a parent span, this request is being ignored, so skip it
58-
const parentSpan = getActiveSpan();
59-
if (!parentSpan) {
60-
return originalHandle.apply(this, [req, res, ...otherArgs]);
49+
function layerHandlePatched(
50+
this: ExpressLayer,
51+
req: ExpressRequest,
52+
res: ExpressResponse,
53+
//oxlint-disable-next-line no-explicit-any
54+
...otherArgs: any[]
55+
) {
56+
// Only create spans when there's an active parent span
57+
// Without a parent span, this request is being ignored, so skip it
58+
const parentSpan = getActiveSpan();
59+
if (!parentSpan) {
60+
return originalHandle.apply(this, [req, res, ...otherArgs]);
61+
}
62+
63+
if (layerPath) storeLayer(req, layerPath);
64+
const storedLayers = getStoredLayers(req);
65+
const isLayerPathStored = !!layerPath;
66+
67+
const constructedRoute = getConstructedRoute(req);
68+
const actualMatchedRoute = getActualMatchedRoute(req, constructedRoute);
69+
70+
options.onRouteResolved?.(actualMatchedRoute);
71+
72+
const metadata = getLayerMetadata(constructedRoute, layer, layerPath);
73+
const name = metadata.attributes[ATTR_EXPRESS_NAME] as string ?? metadata.name;
74+
const type = metadata.attributes[ATTR_EXPRESS_TYPE] as ExpressLayerType;
75+
const attributes: SpanAttributes = Object.assign(metadata.attributes, {
76+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.express',
77+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: `${type}.express`,
78+
});
79+
if (actualMatchedRoute) {
80+
attributes[ATTR_HTTP_ROUTE] = actualMatchedRoute;
81+
}
82+
83+
// verify against the config if the layer should be ignored
84+
if (isLayerIgnored(metadata.name, type, options)) {
85+
// XXX: the isLayerPathStored guard here is *not* present in the
86+
// original @opentelemetry/instrumentation-express impl, but was
87+
// suggested by the Sentry code review bot. It appears to correctly
88+
// prevent improper layer calculation in the case where there's a
89+
// middleware without a layerPath argument. It's unclear whether
90+
// that's possible, or if any existing code depends on that "bug".
91+
if (isLayerPathStored && type === ExpressLayerType_MIDDLEWARE) {
92+
storedLayers.pop();
6193
}
62-
63-
if (layerPath) storeLayer(req, layerPath);
64-
const storedLayers = getStoredLayers(req);
65-
const isLayerPathStored = !!layerPath;
66-
67-
const constructedRoute = getConstructedRoute(req);
68-
const actualMatchedRoute = getActualMatchedRoute(req, constructedRoute);
69-
70-
options.onRouteResolved?.(actualMatchedRoute);
71-
72-
const metadata = getLayerMetadata(constructedRoute, layer, layerPath);
73-
const type = metadata.attributes[ATTR_EXPRESS_TYPE] as ExpressLayerType;
74-
const attributes: SpanAttributes = Object.assign(metadata.attributes, {
75-
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.express',
76-
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: `${type}.express`,
77-
});
78-
if (actualMatchedRoute) {
79-
attributes[ATTR_HTTP_ROUTE] = actualMatchedRoute;
80-
}
81-
82-
// verify against the config if the layer should be ignored
83-
if (isLayerIgnored(metadata.name, type, options)) {
84-
// XXX: the isLayerPathStored guard here is *not* present in the
85-
// original @opentelemetry/instrumentation-express impl, but was
86-
// suggested by the Sentry code review bot. It appears to correctly
87-
// prevent improper layer calculation in the case where there's a
88-
// middleware without a layerPath argument. It's unclear whether
89-
// that's possible, or if any existing code depends on that "bug".
90-
if (isLayerPathStored && type === ExpressLayerType_MIDDLEWARE) {
91-
storedLayers.pop();
92-
}
93-
return originalHandle.apply(this, [req, res, ...otherArgs]);
94+
return originalHandle.apply(this, [req, res, ...otherArgs]);
95+
}
96+
97+
const spanName = getSpanName(
98+
{
99+
request: req,
100+
layerType: type,
101+
route: constructedRoute,
102+
},
103+
name,
104+
);
105+
return startSpanManual({ name: spanName, attributes }, span => {
106+
let spanHasEnded = false;
107+
// TODO: Fix router spans (getRouterPath does not work properly) to
108+
// have useful names before removing this branch
109+
if (metadata.attributes[ATTR_EXPRESS_TYPE] === ExpressLayerType_ROUTER) {
110+
span.end();
111+
spanHasEnded = true;
94112
}
95-
96-
const spanName = getSpanName(
97-
{
98-
request: req,
99-
layerType: type,
100-
route: constructedRoute,
101-
},
102-
metadata.name,
103-
);
104-
return startSpanManual({ name: spanName, attributes }, span => {
105-
// Also update the name, we don't need to "middleware - " prefix
106-
const name = attributes[ATTR_EXPRESS_NAME];
107-
if (typeof name === 'string') {
108-
// should this be updateSpanName?
109-
span.updateName(name);
110-
}
111-
112-
let spanHasEnded = false;
113-
// TODO: Fix router spans (getRouterPath does not work properly) to
114-
// have useful names before removing this branch
115-
if (metadata.attributes[ATTR_EXPRESS_TYPE] === ExpressLayerType_ROUTER) {
116-
span.end();
113+
// listener for response.on('finish')
114+
const onResponseFinish = () => {
115+
if (spanHasEnded === false) {
117116
spanHasEnded = true;
117+
span.end();
118118
}
119-
// listener for response.on('finish')
120-
const onResponseFinish = () => {
119+
};
120+
121+
// verify we have a callback
122+
for (let i = 0; i < otherArgs.length; i++) {
123+
const callback = otherArgs[i] as Function;
124+
if (typeof callback !== 'function') continue;
125+
126+
//oxlint-disable-next-line no-explicit-any
127+
otherArgs[i] = function (...args: any[]) {
128+
// express considers anything but an empty value, "route" or "router"
129+
// passed to its callback to be an error
130+
const maybeError = args[0];
131+
const isError = ![undefined, null, 'route', 'router'].includes(maybeError);
132+
if (!spanHasEnded && isError) {
133+
const [_, message] = asErrorAndMessage(maybeError);
134+
// intentionally do not record the exception here, because
135+
// the error handler we assign does that
136+
span.setStatus({
137+
code: SPAN_STATUS_ERROR,
138+
message,
139+
});
140+
}
141+
121142
if (spanHasEnded === false) {
122143
spanHasEnded = true;
144+
res.removeListener('finish', onResponseFinish);
123145
span.end();
124146
}
147+
if (!(req.route && isError) && isLayerPathStored) {
148+
storedLayers.pop();
149+
}
150+
// execute the callback back in the parent's scope, so that
151+
// we bubble up each level as next() is called.
152+
return withActiveSpan(parentSpan, () => callback.apply(this, args));
125153
};
154+
break;
155+
}
126156

127-
// verify we have a callback
128-
for (let i = 0; i < otherArgs.length; i++) {
129-
const callback = otherArgs[i] as Function;
130-
if (typeof callback !== 'function') continue;
131-
132-
//oxlint-disable-next-line no-explicit-any
133-
otherArgs[i] = function (...args: any[]) {
134-
// express considers anything but an empty value, "route" or "router"
135-
// passed to its callback to be an error
136-
const maybeError = args[0];
137-
const isError = ![undefined, null, 'route', 'router'].includes(maybeError);
138-
if (!spanHasEnded && isError) {
139-
const [_, message] = asErrorAndMessage(maybeError);
140-
// intentionally do not record the exception here, because
141-
// the error handler we assign does that
142-
span.setStatus({
143-
code: SPAN_STATUS_ERROR,
144-
message,
145-
});
146-
}
147-
148-
if (spanHasEnded === false) {
149-
spanHasEnded = true;
150-
res.removeListener('finish', onResponseFinish);
151-
span.end();
152-
}
153-
if (!(req.route && isError) && isLayerPathStored) {
154-
storedLayers.pop();
155-
}
156-
// execute the callback back in the parent's scope, so that
157-
// we bubble up each level as next() is called.
158-
return withActiveSpan(parentSpan, () => callback.apply(this, args));
159-
};
160-
break;
157+
try {
158+
return originalHandle.apply(this, [req, res, ...otherArgs]);
159+
} catch (anyError) {
160+
const [_, message] = asErrorAndMessage(anyError);
161+
// intentionally do not record the exception here, because
162+
// the error handler we assign does that
163+
span.setStatus({
164+
code: SPAN_STATUS_ERROR,
165+
message,
166+
});
167+
throw anyError;
168+
/* v8 ignore next - it sees the block end at the throw */
169+
} finally {
170+
// At this point if the callback wasn't called, that means
171+
// either the layer is asynchronous (so it will call the
172+
// callback later on) or that the layer directly ends the
173+
// http response, so we'll hook into the "finish" event to
174+
// handle the later case.
175+
if (!spanHasEnded) {
176+
res.once('finish', onResponseFinish);
161177
}
178+
}
179+
});
180+
}
162181

163-
try {
164-
return originalHandle.apply(this, [req, res, ...otherArgs]);
165-
} catch (anyError) {
166-
const [_, message] = asErrorAndMessage(anyError);
167-
// intentionally do not record the exception here, because
168-
// the error handler we assign does that
169-
span.setStatus({
170-
code: SPAN_STATUS_ERROR,
171-
message,
172-
});
173-
throw anyError;
174-
/* v8 ignore next - it sees the block end at the throw */
175-
} finally {
176-
// At this point if the callback wasn't called, that means
177-
// either the layer is asynchronous (so it will call the
178-
// callback later on) or that the layer directly ends the
179-
// http response, so we'll hook into the "finish" event to
180-
// handle the later case.
181-
if (!spanHasEnded) {
182-
res.once('finish', onResponseFinish);
183-
}
184-
}
185-
});
186-
},
182+
// `handle` isn't just a regular function in some cases. It also contains
183+
// some properties holding metadata and state so we need to proxy them
184+
// through through patched function. Use a for-in to also pick up properties
185+
// that other libraries might add to the prototype before we instrument.
186+
// ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1950
187+
// ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2271
188+
// oxlint-disable-next-line guard-for-in
189+
for (const key in originalHandle as Function & Record<string, unknown>) {
190+
// skip standard function prototype fields that both have
191+
if (key in layerHandlePatched) {
192+
continue;
193+
}
194+
Object.defineProperty(layerHandlePatched, key, {
195+
get() {
196+
return originalHandle[key];
197+
},
198+
set(value) {
199+
originalHandle[key] = value;
200+
},
201+
});
202+
}
203+
204+
Object.defineProperty(layer, 'handle', {
205+
enumerable: true,
206+
configurable: true,
207+
writable: true,
208+
value: layerHandlePatched,
187209
});
188210
}

‎packages/core/src/integrations/express/utils.ts‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
ExpressLayerType_REQUEST_HANDLER,
2626
ExpressLayerType_ROUTER,
2727
} from './types';
28+
import { isMatchingPattern } from '../../utils/string';
2829

2930
/**
3031
* Check whether the given obj match pattern
@@ -33,15 +34,8 @@ import {
3334
* @param pattern Match pattern
3435
*/
3536
export const satisfiesPattern = (constant: string, pattern: IgnoreMatcher): boolean => {
36-
if (typeof pattern === 'string') {
37-
return pattern === constant;
38-
} else if (pattern instanceof RegExp) {
39-
return pattern.test(constant);
40-
} else if (typeof pattern === 'function') {
41-
return pattern(constant);
42-
} else {
43-
throw new TypeError('Pattern is unsupported datatype');
44-
}
37+
// XXX replace calls to satisfiesPattern with this:
38+
return isMatchingPattern(constant, pattern, true);
4539
};
4640

4741
/**

‎packages/core/src/utils/string.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ export function safeJoin(input: unknown[], delimiter?: string): string {
104104
*/
105105
export function isMatchingPattern(
106106
value: string,
107-
pattern: RegExp | string,
107+
pattern: RegExp | string | ((value: string) => boolean),
108108
requireExactStringMatch: boolean = false,
109109
): boolean {
110110
if (!isString(value)) {
@@ -117,6 +117,9 @@ export function isMatchingPattern(
117117
if (isString(pattern)) {
118118
return requireExactStringMatch ? value === pattern : value.includes(pattern);
119119
}
120+
if (typeof pattern === 'function') {
121+
return pattern(value);
122+
}
120123

121124
return false;
122125
}

‎packages/core/test/lib/integrations/express/types.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
import * as types from '../../../../src/integrations/express/types';
22
import { describe, it, expect } from 'vitest';
33

4-
// this is mostly just a types-bag, but it does have some keys and such.
4+
// this is mostly just a types-bag, but it does have some constant keys
55
describe('types', () => {
6-
it('exports some stuff', () => {
6+
it('exports several constants', () => {
77
const { kLayerPatched, ...vals } = types;
88
expect(String(kLayerPatched)).toBe('Symbol(express-layer-patched)');
99
expect(vals).toStrictEqual({

‎packages/core/test/lib/utils/string.test.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, test } from 'vitest';
1+
import { describe, expect, test, vi } from 'vitest';
22
import { isMatchingPattern, stringMatchesSomePattern, truncate } from '../../../src/utils/string';
33

44
describe('truncate()', () => {
@@ -57,6 +57,15 @@ describe('isMatchingPattern()', () => {
5757
expect(isMatchingPattern('', '')).toEqual(true);
5858
});
5959

60+
test('should call a method that returns boolean result', () => {
61+
const testTrue = vi.fn(() => true);
62+
const testFalse = vi.fn(() => false);
63+
expect(isMatchingPattern('x', testTrue)).toEqual(true);
64+
expect(testTrue).toHaveBeenCalledExactlyOnceWith('x');
65+
expect(isMatchingPattern('y', testFalse)).toEqual(false);
66+
expect(testFalse).toHaveBeenCalledExactlyOnceWith('y');
67+
});
68+
6069
test('should bail out with false when given non-string value', () => {
6170
expect(isMatchingPattern(null as any, 'foo')).toEqual(false);
6271
expect(isMatchingPattern(undefined as any, 'foo')).toEqual(false);

0 commit comments

Comments
 (0)