Skip to content

Commit

Permalink
feat(node): Ensure fastify spans have better data (#12106)
Browse files Browse the repository at this point in the history 
This ensures we have correct op, name & origin for all fastify
middleware spans.

Sadly, we have no good hook in fastify to handle all spans it emits -
only `request_handler` spans are passed to `requestHook` 😬 so I opted to
add a `spanStart` hook to handle this. Since it would suck to always
register this, even without fastify being used, I put this into the
fastify error handler - kind of mixing concerns there but I'd say it's
fine for this case (as it just enhances the data).

I had some issues running this locally, so I changed the playwright
config to not be TS which is IMHO ok and simplifies the setup a bit...
  • Loading branch information
@mydea
mydea committed May 17, 2024
1 parent b43779e commit 5317793
Show file tree
Hide file tree
Showing 5 changed files with 141 additions and 93 deletions.
2 changes: 1 addition & 1 deletion dev-packages/e2e-tests/test-applications/node-fastify/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
},
"devDependencies": {
"@sentry-internal/event-proxy-server": "link:../../../event-proxy-server",
"@playwright/test": "^1.38.1"
"@playwright/test": "^1.44.0"
},
"volta": {
"extends": "../../package.json"
Expand Down
3 changes: 1 addition & 2 deletions ...cations/node-fastify/playwright.config.ts → ...ations/node-fastify/playwright.config.mjs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { PlaywrightTestConfig } from '@playwright/test';
import { devices } from '@playwright/test';

const fastifyPort = 3030;
Expand All @@ -7,7 +6,7 @@ const eventProxyPort = 3031;
/**
* See https://playwright.dev/docs/test-configuration.
*/
const config: PlaywrightTestConfig = {
const config = {
testDir: './tests',
/* Maximum time one test can run for. */
timeout: 150_000,
Expand Down
136 changes: 72 additions & 64 deletions dev-packages/e2e-tests/test-applications/node-fastify/tests/transactions.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,70 +55,6 @@ test('Sends an API route transaction', async ({ baseURL }) => {

expect(transactionEvent).toEqual(
expect.objectContaining({
spans: [
{
data: {
'plugin.name': 'fastify -> sentry-fastify-error-handler',
'fastify.type': 'middleware',
'hook.name': 'onRequest',
'otel.kind': 'INTERNAL',
'sentry.origin': 'manual',
},
description: 'middleware - fastify -> sentry-fastify-error-handler',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: expect.any(String),
origin: 'manual',
},
{
data: {
'plugin.name': 'fastify -> sentry-fastify-error-handler',
'fastify.type': 'request_handler',
'http.route': '/test-transaction',
'otel.kind': 'INTERNAL',
'sentry.origin': 'auto.http.otel.fastify',
},
description: 'request handler - fastify -> sentry-fastify-error-handler',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: expect.any(String),
origin: 'auto.http.otel.fastify',
},
{
data: {
'otel.kind': 'INTERNAL',
'sentry.origin': 'manual',
},
description: 'test-span',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: expect.any(String),
origin: 'manual',
},
{
data: {
'otel.kind': 'INTERNAL',
'sentry.origin': 'manual',
},
description: 'child-span',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: expect.any(String),
origin: 'manual',
},
],
transaction: 'GET /test-transaction',
type: 'transaction',
transaction_info: {
Expand All @@ -127,6 +63,78 @@ test('Sends an API route transaction', async ({ baseURL }) => {
}),
);

const spans = transactionEvent.spans || [];

expect(spans).toContainEqual({
data: {
'plugin.name': 'fastify -> sentry-fastify-error-handler',
'fastify.type': 'middleware',
'hook.name': 'onRequest',
'otel.kind': 'INTERNAL',
'sentry.origin': 'auto.http.otel.fastify',
'sentry.op': 'middleware.fastify',
},
description: 'sentry-fastify-error-handler',
op: 'middleware.fastify',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: expect.any(String),
origin: 'auto.http.otel.fastify',
});

expect(spans).toContainEqual({
data: {
'plugin.name': 'fastify -> sentry-fastify-error-handler',
'fastify.type': 'request_handler',
'http.route': '/test-transaction',
'otel.kind': 'INTERNAL',
'sentry.op': 'request_handler.fastify',
'sentry.origin': 'auto.http.otel.fastify',
},
description: 'sentry-fastify-error-handler',
op: 'request_handler.fastify',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: expect.any(String),
origin: 'auto.http.otel.fastify',
});

expect(spans).toContainEqual({
data: {
'otel.kind': 'INTERNAL',
'sentry.origin': 'manual',
},
description: 'test-span',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: expect.any(String),
origin: 'manual',
});

expect(spans).toContainEqual({
data: {
'otel.kind': 'INTERNAL',
'sentry.origin': 'manual',
},
description: 'child-span',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: expect.any(String),
origin: 'manual',
});

await expect
.poll(
async () => {
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/e2e-tests/test-applications/node-fastify/tsconfig.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@
"strict": true,
"outDir": "dist"
},
"include": ["*.ts"]
"include": ["./src/*.ts"]
}
91 changes: 66 additions & 25 deletions packages/node/src/integrations/tracing/fastify.ts
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,39 @@
import { isWrapped } from '@opentelemetry/core';
import { FastifyInstrumentation } from '@opentelemetry/instrumentation-fastify';
import { captureException, defineIntegration, getIsolationScope, isEnabled } from '@sentry/core';
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
captureException,
defineIntegration,
getClient,
getIsolationScope,
isEnabled,
spanToJSON,
} from '@sentry/core';
import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry';
import type { IntegrationFn } from '@sentry/types';
import type { IntegrationFn, Span } from '@sentry/types';
import { consoleSandbox } from '@sentry/utils';

import { addOriginToSpan } from '../../utils/addOriginToSpan';
// We inline the types we care about here
interface Fastify {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
register: (plugin: any) => void;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
addHook: (hook: string, handler: (request: any, reply: any, error: Error) => void) => void;
}

/**
* Minimal request type containing properties around route information.
* Works for Fastify 3, 4 and presumably 5.
*/
interface FastifyRequestRouteInfo {
// since fastify@4.10.0
routeOptions?: {
url?: string;
method?: string;
};
routerPath?: string;
}

const _fastifyIntegration = (() => {
return {
Expand All @@ -14,7 +42,7 @@ const _fastifyIntegration = (() => {
addOpenTelemetryInstrumentation(
new FastifyInstrumentation({
requestHook(span) {
addOriginToSpan(span, 'auto.http.otel.fastify');
addFastifySpanAttributes(span);
},
}),
);
Expand All @@ -29,27 +57,6 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

// We inline the types we care about here
interface Fastify {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
register: (plugin: any) => void;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
addHook: (hook: string, handler: (request: any, reply: any, error: Error) => void) => void;
}

/**
* Minimal request type containing properties around route information.
* Works for Fastify 3, 4 and presumably 5.
*/
interface FastifyRequestRouteInfo {
// since fastify@4.10.0
routeOptions?: {
url?: string;
method?: string;
};
routerPath?: string;
}

/**
* Setup an error handler for Fastify.
*/
Expand Down Expand Up @@ -84,6 +91,16 @@ export function setupFastifyErrorHandler(fastify: Fastify): void {

fastify.register(plugin);

// Sadly, middleware spans do not go through `requestHook`, so we handle those here
// We register this hook in this method, because if we register it in the integration `setup`,
// it would always run even for users that are not even using fastify
const client = getClient();
if (client) {
client.on('spanStart', span => {
addFastifySpanAttributes(span);
});
}

if (!isWrapped(fastify.addHook) && isEnabled()) {
consoleSandbox(() => {
// eslint-disable-next-line no-console
Expand All @@ -93,3 +110,27 @@ export function setupFastifyErrorHandler(fastify: Fastify): void {
});
}
}

function addFastifySpanAttributes(span: Span): void {
const attributes = spanToJSON(span).data || {};

// this is one of: middleware, request_handler
const type = attributes['fastify.type'];

// If this is already set, or we have no fastify span, no need to process again...
if (attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !type) {
return;
}

span.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.fastify',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: `${type}.fastify`,
});

// Also update the name, we don't need to "middleware - " prefix
const name = attributes['fastify.name'] || attributes['plugin.name'] || attributes['hook.name'];
if (typeof name === 'string') {
// Also remove `fastify -> ` prefix
span.updateName(name.replace(/^fastify -> /, ''));
}
}

0 comments on commit 5317793

Please sign in to comment.