feat(instrumentation-aws-sdk): add bedrock extension #1

anuraaga · 2025-01-30T06:57:14Z

@codefromthecrypt @trentm This is a PR to my fork for initial bedrock extension for JS. I can send it upstream if it generally looks OK

codefromthecrypt

Will leave up to @trentm about js style, but this looks to be the same pattern as Python, where the aws sdk instrumentation is extended to include genai spans when working in bedrock.

Probably a good idea to make an issue like the "toehold" on in python in the genai instrumentation repo and mention this once it becomes a real pr.

Mentioning this beforehand could be confusing to non developers who may miss this is a sort of 'preflight' PR made to reduce back and forth in otel upstream repo.

trentm

Nice.

I have a number of nits/JS/style suggestions below.

A more general question: It is intentional (for now at least) that this isn't getting into emitting events (per https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/gen-ai-events.md) and metrics?

trentm · 2025-02-04T23:32:23Z

plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/bedrock-runtime.ts

+const SEMATTRS_GEN_AI_RESPONSE_FINISH_REASONS =
+  'gen_ai.response.finish_reasons';
+
+export class BedrockRuntimeExtension implements ServiceExtension {


nit:

Suggested change

export class BedrockRuntimeExtension implements ServiceExtension {

export class BedrockRuntimeServiceExtension implements ServiceExtension {

The naming pattern from the others seems to be [ClientName]ServiceExtension.

trentm · 2025-02-04T23:33:48Z

plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/bedrock-runtime.ts

+const SEMATTRS_GEN_AI_USAGE_INPUT_TOKENS = 'gen_ai.usage.input_tokens';
+const SEMATTRS_GEN_AI_USAGE_OUTPUT_TOKENS = 'gen_ai.usage.output_tokens';
+const SEMATTRS_GEN_AI_RESPONSE_FINISH_REASONS =
+  'gen_ai.response.finish_reasons';


FYI: We are close to having a pattern and a tool (open-telemetry#2669) to help generate a local src/semconv.ts to hold unstable semconv constants. However, I don't think this PR should get into handling that for all of instrumentation-aws-sdk. Better to do that in a separate PR. In other words: this is all good here.

We could update this to the newer-style constant names, if you like, e.g.:

const ATTR_GEN_AI_SYSTEM = 'gen_ai.system'; const ATTR_GEN_AI_OPERATION_NAME = 'gen_ai.operation.name';

etc. https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions/#migrating-from-semattrs_-semresattrs_- for details on that deprecation.
However, no need to do so now. Happy to help with that migration.

trentm · 2025-02-04T23:49:02Z

plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/bedrock-runtime.ts

+  ): RequestMetadata {
+    let spanName: string | undefined;
+    const spanAttributes: Attributes = {
+      [SEMATTRS_GEN_AI_SYSTEM]: 'bedrock',


Per open-telemetry/semantic-conventions#1574 (in semconv v1.29.0) the name for this is 'aws.bedrock'.

Suggested change

[SEMATTRS_GEN_AI_SYSTEM]: 'bedrock',

[SEMATTRS_GEN_AI_SYSTEM]: 'aws.bedrock',

trentm · 2025-02-04T23:54:32Z

plugins/node/opentelemetry-instrumentation-aws-sdk/test/bedrock.test.ts

+  ATTR_GEN_AI_USAGE_INPUT_TOKENS,
+  ATTR_GEN_AI_USAGE_OUTPUT_TOKENS,
+  ATTR_GEN_AI_RESPONSE_FINISH_REASONS,
+} from '@opentelemetry/semantic-conventions/incubating';


I see that you are using the newer form of exported constants here.

We have a (recent) rule that "thou shalt not import the .../incubating" entry-point. Instead the recommendation is to have a copy of used attributes in a local src/semconv.ts. Docs on this here: https://github.com/open-telemetry/opentelemetry-js/blob/main/semantic-conventions/README.md#unstable-semconv

I pointed to a tool to help generate this semconv.ts file above. My PR to add that tool is here: open-telemetry#2669
(I'm having trouble getting reviews on my semconv-related PRs of late, because of the OTel JS focus on an "SDK 2.0" milestone right now.)

Yeah I imported here only for the test, which checks for typos in the attribute names of the local definitions. With a tool to mostly guarantee no typos it might not matter, but is it intended to forbid importing incubating in tests?

but is it intended to forbid importing incubating in tests?

Heh, I figured you might ask.
I don't actually recall. It took a lot of back and forth to settle on the current advice. (Discussion was at open-telemetry/opentelemetry-js#5182).

My inclination is not use it in tests either. The scenario I'm thinking of is that, say a handful of instrumentations in opentelemetry-js-contrib.git are using .../incubating in their tests. Then it comes to a time when someone is updating the version of @opentelemetry/semantic-conventions used in the repo. This update would want to update the verison for all packages in the repo, because one of the goals of not ever pinning the dep of this package is to avoid having many copies of it installed in a user's node_modules/....

So if that update introduces breaking changes in the incubating entry-point (which is explicitly allowed), then it is the person updating the semconv dep for the whole repo that needs to go work through any package tests that happen to break.

As you say, if the src/semconv.ts files are generated with a tool, then there shouldn't be much/any need to be double-checking them by comparing with the incubating exports in tests.

Also, FWIW, the PR that I have to add a lint step to check on our intended usage of the semconv dep will break on test files attempting to use the incubating endpoint: open-telemetry#2569

trentm · 2025-02-04T23:55:53Z

plugins/node/opentelemetry-instrumentation-aws-sdk/test/bedrock.test.ts

nit: I'd consider renaming this file "bedrock-runtime.test.ts" to keep the name match with the @aws-sdk/bedrock-runtime package name.

trentm · 2025-02-05T00:02:19Z

plugins/node/opentelemetry-instrumentation-aws-sdk/test/bedrock.test.ts

+  nockBack.fixtures = path.join(__dirname, 'mock-responses');
+  if (!process.env.NOCK_BACK_MODE) {
+    nockBack.setMode('record');
+  }


Not sure if it would be worth a short explanation of NOCK_BACK_MODE usage for re-generating the mock response file for this test. Those unfamiliar with nock.back will need to do some digging/reading. There isn't a current place in the README for this and it is internal detail stuff. I think a block comment at/near the top of the file would be useful.

trentm · 2025-02-05T00:08:35Z

plugins/node/opentelemetry-instrumentation-aws-sdk/test/bedrock.test.ts

+  ConverseCommand,
+  ConversationRole,
+} from '@aws-sdk/client-bedrock-runtime';
+import * as path from 'node:path';


Suggested change

import * as path from 'node:path';

import * as path from 'path';

Support for the node: prefix for builtin modules was added in v14.18.0, but "engines" for this package is currently "14", so it could fail for the poor sap still using the ancient early v14 version.

trentm · 2025-02-05T00:21:39Z

package-lock.json

ZOMG big change here.
I think this is almost certainly because the added @aws-sdk/client-bedrock-runtime dep is of a much more recent generation than all the other deps. That means we'll be getting between 2 and 7 installations of all the transitive deps.

"@aws-sdk/client-bedrock-runtime": "3.587.0", "@aws-sdk/client-dynamodb": "3.85.0", "@aws-sdk/client-kinesis": "3.85.0", "@aws-sdk/client-lambda": "3.85.0", "@aws-sdk/client-s3": "3.85.0", "@aws-sdk/client-sns": "3.85.0", "@aws-sdk/client-sqs": "3.85.0",

AFAIK there is no reason to have these other aws-sdk deps sitting at ancient versions. We should bump them up... perhaps even in this change.

Or if that gets gross, I'd be happy to do a PR to upstream that bumps the aws-sdk/client-* deps to recent versions.

Playing locally I upgraded all these deps as follows:

First apply this diff:

diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/package.json b/plugins/node/opentelemetry-instrumentation-aws-sdk/package.json index 80a8d7cf..2f2fad89 100644 --- a/plugins/node/opentelemetry-instrumentation-aws-sdk/package.json +++ b/plugins/node/opentelemetry-instrumentation-aws-sdk/package.json @@ -50,14 +50,14 @@ "@opentelemetry/semantic-conventions": "^1.27.0" }, "devDependencies": { - "@aws-sdk/client-bedrock-runtime": "3.587.0", - "@aws-sdk/client-dynamodb": "3.85.0", - "@aws-sdk/client-kinesis": "3.85.0", - "@aws-sdk/client-lambda": "3.85.0", - "@aws-sdk/client-s3": "3.85.0", - "@aws-sdk/client-sns": "3.85.0", - "@aws-sdk/client-sqs": "3.85.0", - "@aws-sdk/types": "3.78.0", + "@aws-sdk/client-bedrock-runtime": "^3.587.0", + "@aws-sdk/client-dynamodb": "^3.85.0", + "@aws-sdk/client-kinesis": "^3.85.0", + "@aws-sdk/client-lambda": "^3.85.0", + "@aws-sdk/client-s3": "^3.85.0", + "@aws-sdk/client-sns": "^3.85.0", + "@aws-sdk/client-sqs": "^3.85.0", + "@aws-sdk/types": "^3.78.0", "@opentelemetry/api": "^1.3.0", "@opentelemetry/contrib-test-utils": "^0.45.0", "@opentelemetry/sdk-trace-base": "^1.8.0", diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts index a3040a15..f5e0ebc5 100644 --- a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts +++ b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts @@ -446,7 +446,11 @@ export class AwsInstrumentation extends InstrumentationBase<AwsSdkInstrumentatio const serviceName = clientConfig?.serviceId ?? removeSuffixFromStringIfExists( - awsExecutionContext.clientName, + // Use 'AWS' as a fallback serviceName. I'm not sure of a case where + // an aws-sdk-v3 client does not have a `clientName`. The relevant + // `@aws-sdk/types` change to `interface HandlerExecutionContext` is + // https://github.com/smithy-lang/smithy-typescript/pull/931 + awsExecutionContext.clientName || 'AWS', 'Client' ); const commandName =

Then update the deps:

cd plugins/node/opentelemetry-instrumentation-aws-sdk npm update $(npm outdated | rg ^@aws-sdk | cut -d' ' -f1 | xargs)

It would be a cleaner change to have me (or someone) do that upgrade of deps in a separate PR to upstream.

codefromthecrypt · 2025-02-05T01:13:04Z

thanks tons for the feedback, which looked to take a fair amount of time but a good investment @trentm!

anuraaga · 2025-02-05T01:15:44Z

A more general question: It is intentional (for now at least) that this isn't getting into emitting events (per https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/gen-ai-events.md) and metrics?

@trentm Yeah, this is meant to be an initial step with still (hopefully) reasonable implementation of required genai attributes on spans. Will follow up with more to keep the diffs smaller

anuraaga · 2025-02-06T04:06:48Z

Thanks @trentm - I have applied all the changes and will go ahead and send this upstream where we can iterate more as needed

anuraaga · 2025-02-06T04:19:09Z

@trentm Having your attention, was wondering if you can help with this mostly unrelated PR to the SDK

open-telemetry/opentelemetry-js#5303

My sense is PRs without auto assigned reviewers never get reviewed so trying to see if some backchanneling can help...

feat(instrumentation-aws-sdk): add bedrock extension

6cb00fa

github-actions bot added the pkg:instrumentation-aws-sdk label Jan 30, 2025

Use minimum bedrock that supports Converse

765c280

codefromthecrypt approved these changes Jan 31, 2025

View reviewed changes

trentm reviewed Feb 5, 2025

View reviewed changes

anuraaga added 2 commits February 6, 2025 11:09

Cleanups

94d7fe4

Update deps

36ca8fd

anuraaga closed this Feb 6, 2025

anuraaga mentioned this pull request Feb 6, 2025

feat(instrumentation-aws-sdk): add bedrock extension to apply gen ai conventions open-telemetry/opentelemetry-js-contrib#2700

Merged

	export class BedrockRuntimeExtension implements ServiceExtension {
	export class BedrockRuntimeServiceExtension implements ServiceExtension {

	[SEMATTRS_GEN_AI_SYSTEM]: 'bedrock',
	[SEMATTRS_GEN_AI_SYSTEM]: 'aws.bedrock',

	import * as path from 'node:path';
	import * as path from 'path';

feat(instrumentation-aws-sdk): add bedrock extension #1

feat(instrumentation-aws-sdk): add bedrock extension #1

Uh oh!

Conversation

anuraaga commented Jan 30, 2025

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

trentm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Feb 5, 2025

Uh oh!

anuraaga commented Feb 5, 2025

Uh oh!

anuraaga commented Feb 6, 2025

Uh oh!

anuraaga commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants