refactor: Added @smithy/smithy-client send subscriber with AWS Bedrock Middleware#3864
refactor: Added @smithy/smithy-client send subscriber with AWS Bedrock Middleware#3864amychisholm03 wants to merge 5 commits intonewrelic:mainfrom
@smithy/smithy-client send subscriber with AWS Bedrock Middleware#3864Conversation
f34d803 to
61a01a7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3864 +/- ##
==========================================
- Coverage 97.82% 97.69% -0.13%
==========================================
Files 471 473 +2
Lines 58670 58830 +160
Branches 1 1
==========================================
+ Hits 57391 57472 +81
- Misses 1279 1358 +79
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@smithy/smithy-client send subscriber with AWS Bedrock Middleware@smithy/smithy-client send subscriber with AWS Bedrock Middleware
jsumners-nr
left a comment
There was a problem hiding this comment.
I have not looked at the code yet. I want address organization first. I understand the thinking behind lib/subscribers/smithy-client/ and lib/subscribers/aws-sdk-v3/. But I think that will lead to some difficulty in understanding where things are when reviewing things in the future.
I'd like to suggest we organize things as so:
lib/subscribers/aws-sdk/lib/subscribers/aws-sdk/v3/lib/subscribers/aws-sdk/v3/bedrock.jslib/subscribers/aws-sdk/smithy-client/
technically the only subscriber is smithy-client. The rest are middleware/helpers. |
Do you disagree with my suggestion? |
Not really. Looking at the current code, I could see the structure as this:
Not wedded to this but that's my take on it. No need for |
| }, | ||
| { | ||
| channelName: 'nr_send', | ||
| module: { name: '@smithy/smithy-client', versionRange: '>=1.0.0 <4.0.0', filePath: 'dist-es/index.js' }, |
There was a problem hiding this comment.
is @aws-sdk/smithy-client no longer valid? We were instrumenting that before
There was a problem hiding this comment.
Like I said in my PR description, @aws-sdk/smithy-client is like a 3-liner package that just exports @smithy/smithy-client, so we only need to subscribe to the later.
There was a problem hiding this comment.
Does it do so across all versions of these libraries that we support?
There was a problem hiding this comment.
For npm run versioned aws-sdk-v3 it appears so.
There was a problem hiding this comment.
I'll test more versions with AGENT_PATH="$(pwd)" ./node_modules/.bin/versioned-tests --patch --print pretty -i 100 --all --strict test/versioned/aws-sdk-v3
There was a problem hiding this comment.
According to our tests:
The minimum version of @aws-sdk/smithy-client we support is 3.47.0. That version's manifest does not reference @smithy/smithy-client:
https://npmx.dev/package-code/@aws-sdk/smithy-client/v/3.47.0/package.json#L21-L25
There was a problem hiding this comment.
At least for @aws-sdk/client-bedrock-runtime >=3.474.0, it does not have @aws-sdk/smithy-client as a dependency, only @smithy/smithy-client. Now with other @aws-sdk/* packages, there may be older versions that used @aws-sdk/smithy-client and adding the subscriber config for that could be addressed in that future PR.
There was a problem hiding this comment.
Are you saying that the subscriber implementation only works with @aws-sdk/* >= v3.474.0?
There was a problem hiding this comment.
No, the smithy client subscriber should instrument Client.send (or _Client.send depending on version) for @smithy/smithy-client: >=1, but it is only being tested currently with the AWS Bedrock middleware which is @aws-sdk/client-bedrock-runtime versions >=3.474.
There was a problem hiding this comment.
Okay, I can't see any direct dependents of 3.47 that we are concerned with. So maybe this isn't a problem.
test/unit/instrumentation/aws-sdk/converse-stream-handler.test.js
Outdated
Show resolved
Hide resolved
Makes sense - I did |
4058bca to
9a98df1
Compare
f7a7ad5 to
bc42729
Compare
|
|
||
| // Enter the segment's context so downstream middleware (e.g. HTTP outbound) | ||
| // creates child segments under this bedrock segment. | ||
| agent.tracer.setSegment({ segment }) |
There was a problem hiding this comment.
you want to avoid manually setting the context. This wrapped middleware should follow our standard processes where you bind the original next function to the context via agent.tracer.bindFunction(next, ctx, true).apply(this, arguments) and then run your afterHookstuff. also by passing intrue`. it will start the function and touch it
Description
This PR migrates the AWS Bedrock instrumentation from the shim-based (monkey-patching) architecture to the subscriber-based architecture.
A standalone bedrock middleware module (
lib/subscribers/aws-sdk-v3/bedrock.js) exports aninit/middleware/configobject that creates segments, sets tracing context, and records LLM events for Bedrock API calls. TheSmithyClientSendSubscriberinlib/subscribers/smithy-client/send.jsuses amiddlewareByClientdispatch map to register this bedrock middleware (plus common header/attribute middleware) when it detects aBedrockRuntimeclient.To avoid double instrumentation during us refactoring the other AWS middlewares, the subscriber skips clients not in
middlewareByClient, and the old shim inlib/instrumentation/aws-sdk/v3/smithy-client.jsskips clients that the subscriber handles. This lets bedrock use the new subscriber path while all other AWS services (SNS, SQS, DynamoDB, Lambda) continue using the existing shim path.Note:
@aws-sdk/smithy-clientjust exports@smithy/smithy-client, so we only need to subscribe to the later.How to Test
Related Issues
Closes #3839
Additional Info
To migrate another service (e.g., SNS, SQS, DynamoDB, Lambda):
lib/subscribers/aws-sdk-v3/<service>.jsexporting{ init, middleware, config }— usebedrock.jsas the templatesend.jsand add the client name to themiddlewareByClientmap (e.g.,SNS: [snsMiddlewareConfig])subscriberHandledClientsinlib/instrumentation/aws-sdk/v3/smithy-client.js, so the old shim skips itsubscriberHandledClientsset can be removed entirely