From 7eba0b88579d96db657f75d6be8dfe4666230db3 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Aug 2024 17:06:45 +0200 Subject: [PATCH 1/3] fix(opentelemetry): Do not overwrite http span name if kind is internal --- .../src/utils/parseSpanDescription.ts | 8 ++++++-- .../test/utils/parseSpanDescription.test.ts | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 6d1c9936899b..5c03dc1610b4 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -163,10 +163,14 @@ 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; + return { op: opParts.join('.'), - description, - source, + description: isClientOrServerKind ? description : name, + source: isClientOrServerKind ? source : 'custom', data, }; } diff --git a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts index cfa1a43094c4..2b1d25dbacff 100644 --- a/packages/opentelemetry/test/utils/parseSpanDescription.test.ts +++ b/packages/opentelemetry/test/utils/parseSpanDescription.test.ts @@ -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); From 96db2adc6cbebdd06c8ad54092bdeaf35ed3542b Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 9 Aug 2024 09:24:03 +0200 Subject: [PATCH 2/3] always infer from auto spans --- .../src/utils/parseSpanDescription.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index 5c03dc1610b4..b600b81f8aec 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -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'; @@ -167,10 +167,18 @@ export function descriptionForHttpMethod( // 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: isClientOrServerKind ? description : name, - source: isClientOrServerKind ? source : 'custom', + description: useInferredDescription ? description : name, + source: useInferredDescription ? source : 'custom', data, }; } From 4b9bfa00e689d44fcc00f5c70ed706d6645d404c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 12 Aug 2024 10:38:52 +0200 Subject: [PATCH 3/3] fix nest test --- .../suites/tracing/nestjs/scenario.ts | 5 ++++- .../node-integration-tests/suites/tracing/nestjs/test.ts | 7 ++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/nestjs/scenario.ts index b75ff4d8a9ef..953619d8d437 100644 --- a/dev-packages/node-integration-tests/suites/tracing/nestjs/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs/scenario.ts @@ -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; @@ -48,6 +48,9 @@ class AppModule {} async function run(): Promise { const app = await NestFactory.create(AppModule); await app.listen(port); + + const { httpAdapter } = app.get(HttpAdapterHost); + Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter)); sendPortToRunner(port); } diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs/test.ts b/dev-packages/node-integration-tests/suites/tracing/nestjs/test.ts index 686c93e1cad6..80570044d64d 100644 --- a/dev-packages/node-integration-tests/suites/tracing/nestjs/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs/test.ts @@ -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': '/', }), }), ]),