-
Notifications
You must be signed in to change notification settings - Fork 616
fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans #1748
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
fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans #1748
Changes from 2 commits
a5acebf
52d09b7
c198332
a37da4a
55ce8d5
648c034
449d159
a61251c
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 |
|---|---|---|
|
|
@@ -21,26 +21,36 @@ import { | |
| } from '@opentelemetry/semantic-conventions'; | ||
| import { | ||
| AwsSdkInstrumentationConfig, | ||
| AwsSdkDynamoDBStatementSerializer, | ||
| NormalizedRequest, | ||
| NormalizedResponse, | ||
| } from '../types'; | ||
|
|
||
| const defaultDynamoDBStatementSerializer: AwsSdkDynamoDBStatementSerializer = () => | ||
| undefined; | ||
|
|
||
| export class DynamodbServiceExtension implements ServiceExtension { | ||
| toArray<T>(values: T | T[]): T[] { | ||
| return Array.isArray(values) ? values : [values]; | ||
| } | ||
|
|
||
| requestPreSpanHook(normalizedRequest: NormalizedRequest): RequestMetadata { | ||
| requestPreSpanHook( | ||
| normalizedRequest: NormalizedRequest, | ||
| config: AwsSdkInstrumentationConfig | ||
| ): RequestMetadata { | ||
| const spanKind: SpanKind = SpanKind.CLIENT; | ||
| let spanName: string | undefined; | ||
| const isIncoming = false; | ||
| const operation = normalizedRequest.commandName; | ||
|
|
||
| const dbStatementSerializer = | ||
| config.dynamoDBStatementSerializer || defaultDynamoDBStatementSerializer; | ||
|
|
||
| const spanAttributes = { | ||
| [SemanticAttributes.DB_SYSTEM]: DbSystemValues.DYNAMODB, | ||
| [SemanticAttributes.DB_NAME]: normalizedRequest.commandInput?.TableName, | ||
| [SemanticAttributes.DB_OPERATION]: operation, | ||
| [SemanticAttributes.DB_STATEMENT]: JSON.stringify( | ||
| [SemanticAttributes.DB_STATEMENT]: dbStatementSerializer( | ||
|
||
| normalizedRequest.commandInput | ||
| ), | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ import { Span } from '@opentelemetry/api'; | |
| import { InstrumentationConfig } from '@opentelemetry/instrumentation'; | ||
| import { SQS } from './aws-sdk.types'; | ||
|
|
||
| export type CommandInput = Record<string, any>; | ||
|
|
||
| /** | ||
| * These are normalized request and response, which are used by both sdk v2 and v3. | ||
| * They organize the relevant data in one interface which can be processed in a | ||
|
|
@@ -25,7 +27,7 @@ import { SQS } from './aws-sdk.types'; | |
| export interface NormalizedRequest { | ||
| serviceName: string; | ||
| commandName: string; | ||
| commandInput: Record<string, any>; | ||
| commandInput: CommandInput; | ||
| region?: string; | ||
| } | ||
| export interface NormalizedResponse { | ||
|
|
@@ -62,6 +64,10 @@ export interface AwsSdkSqsProcessCustomAttributeFunction { | |
| (span: Span, sqsProcessInfo: AwsSdkSqsProcessHookInformation): void; | ||
| } | ||
|
|
||
| export type AwsSdkDynamoDBStatementSerializer = ( | ||
| commandInput: CommandInput | ||
| ) => string | undefined; | ||
|
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. I wonder if the serializer should also get the This can also be added in the future if someone brings up the need.
Contributor
Author
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. I like the suggestion. As pointed out elsewhere, the sanitize function is supplied as part of the public API so I think including |
||
|
|
||
| export interface AwsSdkInstrumentationConfig extends InstrumentationConfig { | ||
| /** hook for adding custom attributes before request is sent to aws */ | ||
| preRequestHook?: AwsSdkRequestCustomAttributeFunction; | ||
|
|
@@ -72,6 +78,9 @@ export interface AwsSdkInstrumentationConfig extends InstrumentationConfig { | |
| /** hook for adding custom attribute when an sqs process span is started */ | ||
| sqsProcessHook?: AwsSdkSqsProcessCustomAttributeFunction; | ||
|
|
||
| /** custom serializer function for the db.statement attribute in DynamoDB spans */ | ||
| dynamoDBStatementSerializer?: AwsSdkDynamoDBStatementSerializer; | ||
|
|
||
| /** | ||
| * Most aws operation use http request under the hood. | ||
| * if http instrumentation is enabled, each aws operation will also create | ||
|
|
||
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.
Should we record this attribute if
config.dynamoDBStatementSerializerisundefinedor if the config function returnedundefined?