Skip to content

Commit

Permalink
fix(opentelemetry): Do not overwrite http span name if kind is intern…
Browse files Browse the repository at this point in the history
…al (#13282)

If the kind of a http span is neither client nor server, it implies it
is most likely being started with `startSpan()` manually, in which case
we rather not want to overwrite the name.
  • Loading branch information
mydea authored Aug 12, 2024
1 parent 16996bb commit 51bbf32
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Sentry.init({
});

import { Controller, Get, Injectable, Module } from '@nestjs/common';
import { NestFactory } from '@nestjs/core';
import { BaseExceptionFilter, HttpAdapterHost, NestFactory } from '@nestjs/core';

const port = 3450;

Expand Down Expand Up @@ -48,6 +48,9 @@ class AppModule {}
async function run(): Promise<void> {
const app = await NestFactory.create(AppModule);
await app.listen(port);

const { httpAdapter } = app.get(HttpAdapterHost);
Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter));
sendPortToRunner(port);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,12 @@ conditionalTest({ min: 16 })('nestjs auto instrumentation', () => {
'nestjs.callback': 'getHello',
'nestjs.controller': 'AppController',
'nestjs.type': 'request_context',
'sentry.op': 'http',
'sentry.op': 'request_context.nestjs',
'sentry.origin': 'auto.http.otel.nestjs',
component: '@nestjs/core',
'http.method': 'GET',
'http.route': '/',
'http.url': '/',
}),
}),
]),
Expand Down
18 changes: 15 additions & 3 deletions packages/opentelemetry/src/utils/parseSpanDescription.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
import type { SpanAttributes, TransactionSource } from '@sentry/types';
import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils';

import { SEMANTIC_ATTRIBUTE_SENTRY_OP } from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from '../semanticAttributes';
import type { AbstractSpan } from '../types';
import { getSpanKind } from './getSpanKind';
Expand Down Expand Up @@ -163,10 +163,22 @@ export function descriptionForHttpMethod(
data['http.fragment'] = fragment;
}

// If the span kind is neither client nor server, we use the original name
// this infers that somebody manually started this span, in which case we don't want to overwrite the name
const isClientOrServerKind = kind === SpanKind.CLIENT || kind === SpanKind.SERVER;

// If the span is an auto-span (=it comes from one of our instrumentations),
// we always want to infer the name
// this is necessary because some of the auto-instrumentation we use uses kind=INTERNAL
const origin = attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] || 'manual';
const isManualSpan = !`${origin}`.startsWith('auto');

const useInferredDescription = isClientOrServerKind || !isManualSpan;

return {
op: opParts.join('.'),
description,
source,
description: useInferredDescription ? description : name,
source: useInferredDescription ? source : 'custom',
data,
};
}
Expand Down
19 changes: 19 additions & 0 deletions packages/opentelemetry/test/utils/parseSpanDescription.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,25 @@ describe('descriptionForHttpMethod', () => {
source: 'route',
},
],
[
'works with basic client GET with SpanKind.INTERNAL',
'GET',
{
[SEMATTRS_HTTP_METHOD]: 'GET',
[SEMATTRS_HTTP_URL]: 'https://www.example.com/my-path',
[SEMATTRS_HTTP_TARGET]: '/my-path',
},
'test name',
SpanKind.INTERNAL,
{
op: 'http',
description: 'test name',
data: {
url: 'https://www.example.com/my-path',
},
source: 'custom',
},
],
])('%s', (_, httpMethod, attributes, name, kind, expected) => {
const actual = descriptionForHttpMethod({ attributes, kind, name }, httpMethod);
expect(actual).toEqual(expected);
Expand Down

0 comments on commit 51bbf32

Please sign in to comment.