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(node): Ensure express spans have better data #12107

Merged
merged 2 commits into from
May 21, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,10 @@ test('Should record a transaction for route with parameters', async ({ request }
'http.route': '/',
'otel.kind': 'INTERNAL',
'sentry.origin': 'auto.http.otel.express',
'sentry.op': 'middleware.express',
},
description: 'middleware - query',
op: 'middleware.express',
description: 'query',
origin: 'auto.http.otel.express',
parent_span_id: expect.any(String),
span_id: expect.any(String),
Expand All @@ -86,8 +88,10 @@ test('Should record a transaction for route with parameters', async ({ request }
'http.route': '/',
'otel.kind': 'INTERNAL',
'sentry.origin': 'auto.http.otel.express',
'sentry.op': 'middleware.express',
},
description: 'middleware - expressInit',
op: 'middleware.express',
description: 'expressInit',
origin: 'auto.http.otel.express',
parent_span_id: expect.any(String),
span_id: expect.any(String),
Expand All @@ -104,8 +108,10 @@ test('Should record a transaction for route with parameters', async ({ request }
'http.route': '/test-transaction/:param',
'otel.kind': 'INTERNAL',
'sentry.origin': 'auto.http.otel.express',
'sentry.op': 'request_handler.express',
},
description: 'request handler - /test-transaction/:param',
op: 'request_handler.express',
description: '/test-transaction/:param',
origin: 'auto.http.otel.express',
parent_span_id: expect.any(String),
span_id: expect.any(String),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"@types/node": "18.15.1",
"express": "4.19.2",
"typescript": "4.9.5",
"zod": "^3.22.4"
"zod": "~3.22.4"
},
"devDependencies": {
"@sentry-internal/event-proxy-server": "link:../../../event-proxy-server",
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
import { expect, test } from '@playwright/test';
import { waitForTransaction } from '@sentry-internal/event-proxy-server';
import axios, { AxiosError } from 'axios';

const authToken = process.env.E2E_TEST_AUTH_TOKEN;
const sentryTestOrgSlug = process.env.E2E_TEST_SENTRY_ORG_SLUG;
const sentryTestProject = process.env.E2E_TEST_SENTRY_TEST_PROJECT;
const EVENT_POLLING_TIMEOUT = 90_000;

test('Sends an API route transaction', async ({ baseURL }) => {
const pageloadTransactionEventPromise = waitForTransaction('node-express', transactionEvent => {
return (
transactionEvent?.contexts?.trace?.op === 'http.server' &&
transactionEvent?.transaction === 'GET /test-transaction'
);
});

await axios.get(`${baseURL}/test-transaction`);

const transactionEvent = await pageloadTransactionEventPromise;
const transactionEventId = transactionEvent.event_id;

expect(transactionEvent.contexts?.trace).toEqual({
data: {
'sentry.source': 'route',
'sentry.origin': 'auto.http.otel.http',
'sentry.op': 'http.server',
'sentry.sample_rate': 1,
url: 'http://localhost:3030/test-transaction',
'otel.kind': 'SERVER',
'http.response.status_code': 200,
'http.url': 'http://localhost:3030/test-transaction',
'http.host': 'localhost:3030',
'net.host.name': 'localhost',
'http.method': 'GET',
'http.scheme': 'http',
'http.target': '/test-transaction',
'http.user_agent': 'axios/1.6.7',
'http.flavor': '1.1',
'net.transport': 'ip_tcp',
'net.host.ip': expect.any(String),
'net.host.port': expect.any(Number),
'net.peer.ip': expect.any(String),
'net.peer.port': expect.any(Number),
'http.status_code': 200,
'http.status_text': 'OK',
'http.route': '/test-transaction',
},
op: 'http.server',
span_id: expect.any(String),
status: 'ok',
trace_id: expect.any(String),
origin: 'auto.http.otel.http',
});

expect(transactionEvent).toEqual(
expect.objectContaining({
transaction: 'GET /test-transaction',
type: 'transaction',
transaction_info: {
source: 'route',
},
}),
);

const spans = transactionEvent.spans || [];

expect(spans).toContainEqual({
data: {
'sentry.origin': 'auto.http.otel.express',
'sentry.op': 'middleware.express',
'http.route': '/',
'express.name': 'query',
'express.type': 'middleware',
'otel.kind': 'INTERNAL',
},
description: 'query',
op: 'middleware.express',
origin: 'auto.http.otel.express',
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),
});

expect(spans).toContainEqual({
data: {
'sentry.origin': 'auto.http.otel.express',
'sentry.op': 'middleware.express',
'http.route': '/',
'express.name': 'expressInit',
'express.type': 'middleware',
'otel.kind': 'INTERNAL',
},
description: 'expressInit',
op: 'middleware.express',
origin: 'auto.http.otel.express',
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),
});

expect(spans).toContainEqual({
data: {
'sentry.origin': 'auto.http.otel.express',
'sentry.op': 'request_handler.express',
'http.route': '/test-transaction',
'express.name': '/test-transaction',
'express.type': 'request_handler',
'otel.kind': 'INTERNAL',
},
description: '/test-transaction',
op: 'request_handler.express',
origin: 'auto.http.otel.express',
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),
});

await expect
.poll(
async () => {
try {
const response = await axios.get(
`https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${transactionEventId}/`,
{ headers: { Authorization: `Bearer ${authToken}` } },
);

return response.status;
} catch (e) {
if (e instanceof AxiosError && e.response) {
if (e.response.status !== 404) {
throw e;
} else {
return e.response.status;
}
} else {
throw e;
}
}
},
{
timeout: EVENT_POLLING_TIMEOUT,
},
)
.toBe(200);
});
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,10 @@ test('Sends an API route transaction', async ({ baseURL }) => {
'http.route': '/test-transaction',
'otel.kind': 'INTERNAL',
'sentry.origin': 'auto.http.otel.express',
'sentry.op': 'request_handler.express',
},
description: 'request handler - /test-transaction',
op: 'request_handler.express',
description: '/test-transaction',
parent_span_id: expect.any(String),
span_id: expect.any(String),
start_timestamp: expect.any(Number),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,17 @@ describe('express tracing experimental', () => {
'express.name': 'corsMiddleware',
'express.type': 'middleware',
}),
description: 'middleware - corsMiddleware',
description: 'corsMiddleware',
op: 'middleware.express',
origin: 'auto.http.otel.express',
}),
expect.objectContaining({
data: expect.objectContaining({
'express.name': '/test/express',
'express.type': 'request_handler',
}),
description: '/test/express',
op: 'request_handler.express',
origin: 'auto.http.otel.express',
}),
]),
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
"build:watch": "lerna run build:watch",
"build:dev:watch": "lerna run build:dev:watch",
"build:types:watch": "ts-node scripts/build-types-watch.ts",
"build:tarball": "lerna run build:tarball",
"build:tarball": "run-s clean:tarballs build:tarballs",
"build:tarballs": "lerna run build:tarball",
"circularDepCheck": "lerna run circularDepCheck",
"clean": "run-s clean:build clean:caches",
"clean:build": "lerna run clean",
Expand Down
22 changes: 21 additions & 1 deletion packages/node/src/integrations/tracing/express.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import type * as http from 'http';
import { ExpressInstrumentation } from '@opentelemetry/instrumentation-express';
import { defineIntegration, getDefaultIsolationScope, isEnabled } from '@sentry/core';
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
defineIntegration,
getDefaultIsolationScope,
isEnabled,
spanToJSON,
} from '@sentry/core';
import { captureException, getClient, getIsolationScope } from '@sentry/core';
import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry';
import type { IntegrationFn } from '@sentry/types';
Expand All @@ -19,6 +25,20 @@ const _expressIntegration = (() => {
new ExpressInstrumentation({
requestHook(span) {
addOriginToSpan(span, 'auto.http.otel.express');

const attributes = spanToJSON(span).data || {};
// this is one of: middleware, request_handler, router
const type = attributes['express.type'];

if (type) {
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, `${type}.express`);
}

// Also update the name, we don't need to "middleware - " prefix
const name = attributes['express.name'];
if (typeof name === 'string') {
span.updateName(name);
}
},
spanNameHook(info, defaultName) {
if (getIsolationScope() === getDefaultIsolationScope()) {
Expand Down
Loading