-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): Emit low-cardinality gen_ai inference span names when streaming #23573
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
base: develop
Are you sure you want to change the base?
Changes from all commits
314f52f
1a889f0
d21d777
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| /* eslint-disable typescript-eslint/no-deprecated */ | ||
| import { | ||
| captureException, | ||
| GEN_AI_INFERENCE_SPAN_NAME_FALLBACK, | ||
| getClient, | ||
| hasSpanStreamingEnabled, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
| SPAN_STATUS_ERROR, | ||
| startSpan, | ||
|
|
@@ -189,8 +192,15 @@ function handleStreamingRequest<T extends unknown[], R>( | |
| isStreamingMethod: boolean, | ||
| ): R | Promise<R> { | ||
| const model = requestAttributes[GEN_AI_REQUEST_MODEL] ?? 'unknown'; | ||
| const client = getClient(); | ||
| const spanConfig = { | ||
| name: `${operationName} ${model}`, | ||
| // With span streaming, omit the `'unknown'` model sentinel so the name stays low-cardinality. | ||
| name: | ||
| (typeof model === 'string' && model !== 'unknown') || !(client && hasSpanStreamingEnabled(client)) | ||
| ? `${operationName} ${model}` | ||
| : operationName !== 'unknown' | ||
| ? operationName | ||
| : GEN_AI_INFERENCE_SPAN_NAME_FALLBACK, | ||
|
Comment on lines
+197
to
+203
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. low: This nearly-identical ternary is repeated quite a lot in this patch (eg, this file again on line 310,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I think the |
||
| op: getGenAiSpanOp(operationName), | ||
| attributes: requestAttributes as Record<string, SpanAttributeValue>, | ||
| }; | ||
|
|
@@ -273,6 +283,7 @@ function instrumentMethod<T extends unknown[], R>( | |
| const operationName = instrumentedMethod.operation || 'unknown'; | ||
| const requestAttributes = extractRequestAttributes(args, operationName); | ||
| const model = requestAttributes[GEN_AI_REQUEST_MODEL] ?? 'unknown'; | ||
| const client = getClient(); | ||
|
|
||
| const params = typeof args[0] === 'object' ? (args[0] as Record<string, unknown>) : undefined; | ||
| const isStreamRequested = Boolean(params?.stream); | ||
|
|
@@ -296,7 +307,13 @@ function instrumentMethod<T extends unknown[], R>( | |
|
|
||
| const instrumentedPromise = startSpan( | ||
| { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This object creation is identical to the one on line 196, so we could probably use a helper function to do both in one place. |
||
| name: `${operationName} ${model}`, | ||
| // With span streaming, omit the `'unknown'` model sentinel so the name stays low-cardinality. | ||
| name: | ||
| (typeof model === 'string' && model !== 'unknown') || !(client && hasSpanStreamingEnabled(client)) | ||
| ? `${operationName} ${model}` | ||
| : operationName !== 'unknown' | ||
| ? operationName | ||
| : GEN_AI_INFERENCE_SPAN_NAME_FALLBACK, | ||
| op: getGenAiSpanOp(operationName), | ||
| attributes: requestAttributes as Record<string, SpanAttributeValue>, | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,9 @@ | |
| import { DEBUG_BUILD } from '../../debug-build'; | ||
| import { | ||
| captureException, | ||
| GEN_AI_INFERENCE_SPAN_NAME_FALLBACK, | ||
| getClient, | ||
| hasSpanStreamingEnabled, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
| SPAN_STATUS_ERROR, | ||
| startSpan, | ||
|
|
@@ -144,9 +147,16 @@ function instrumentMethod<T extends unknown[], R>( | |
|
|
||
| const params = args[0] as Record<string, unknown> | undefined; | ||
| const isStreamRequested = params && typeof params === 'object' && params.stream === true; | ||
| const client = getClient(); | ||
|
|
||
| const spanConfig = { | ||
| name: `${operationName} ${model}`, | ||
| // With span streaming, omit the `'unknown'` model sentinel so the name stays low-cardinality. | ||
| name: | ||
| (typeof model === 'string' && model !== 'unknown') || !(client && hasSpanStreamingEnabled(client)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the |
||
| ? `${operationName} ${model}` | ||
| : operationName !== 'unknown' | ||
| ? operationName | ||
| : GEN_AI_INFERENCE_SPAN_NAME_FALLBACK, | ||
| op: getGenAiSpanOp(operationName), | ||
| attributes: requestAttributes as Record<string, SpanAttributeValue>, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| import { | ||
| _INTERNAL_shouldSkipAiProviderWrapping, | ||
| GEN_AI_INFERENCE_SPAN_NAME_FALLBACK, | ||
| getClient, | ||
| hasSpanStreamingEnabled, | ||
| isObjectLike, | ||
| SPAN_STATUS_ERROR, | ||
| startSpan, | ||
|
|
@@ -43,6 +46,7 @@ function instrumentRun( | |
| const operationName = getOperationName(inputs); | ||
| const requestAttributes = extractRequestAttributes(model, inputs, operationName); | ||
| const modelName = typeof model === 'string' ? model : 'unknown'; | ||
| const client = getClient(); | ||
|
|
||
| const isStreamRequested = | ||
| !!inputs && typeof inputs === 'object' && (inputs as { stream?: unknown }).stream === true; | ||
|
|
@@ -52,7 +56,13 @@ function instrumentRun( | |
| (runOptions.returnRawResponse === true || runOptions.websocket === true); | ||
|
|
||
| const spanConfig = { | ||
| name: `${operationName} ${modelName}`, | ||
| // With span streaming, omit the `'unknown'` model sentinel so the name stays low-cardinality. | ||
| name: | ||
| (typeof modelName === 'string' && modelName !== 'unknown') || !(client && hasSpanStreamingEnabled(client)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, |
||
| ? `${operationName} ${modelName}` | ||
| : operationName !== 'unknown' | ||
| ? operationName | ||
| : GEN_AI_INFERENCE_SPAN_NAME_FALLBACK, | ||
| op: `gen_ai.${operationName}`, | ||
| attributes: requestAttributes, | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for this to be an empty string? If so, we could have a span name like
'chat '(with a trailing space). I think if this is||instead, it dodges the issue.