Skip to content
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

[Security solution] Bedrock streaming and token tracking #170815

Merged
merged 38 commits into from
Nov 16, 2023

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Nov 7, 2023

Summary

Part 2 of 3 of Streaming in Security AI Assistant (Part 1)

  1. Implement streaming for the Bedrock connector
  2. Add invokeStream subaction to OpenAI and Bedrock. Handles standardizing our assistant stream request to OpenAI and Bedrock formats, and parses the response from the OpenAI and Bedrock formats to a simple string for the consumer to stream
  3. Adds token tracking for Bedrock, streaming (invokeStream) and non streaming (run, test), in action_executor.ts
    • Note: the invokeAI method of Bedrock does not have token tracking. I plan remove that method in part 3 of 3 of the streaming work. At that time I will add a link to the token tracking dashboard form the Bedrock connector edit, like we have for OpenAI

package.json changes

I could not find documentation on how to handle the Bedrock codec. I did find awsdocs/aws-doc-sdk-examples#5541 that we can track for when documentation should be added.

I followed the bedrock extension in the langchainjs repo here: https://github.com/langchain-ai/langchainjs/pull/2367/files#diff-ead6be41360b3bf1c4559ea4d0069a015b69a57d5f8c51d0cce73253fde30aa2R246-R268. This is how I found that we'd need these packages: @smithy/eventstream-codec, @smithy/util-utf

To test:

  1. Create Bedrock connector with Bedrock credentials and assign to your security AI assistant conversation
  2. Ensure langchain is not enabled. Knowledge base should be off:
Screenshot 2023-11-14 at 10 20 54 AM
  1. Send message to the connector. Test that:
    • response streams
    • "Stop generating" button works
    • "Regenerate" button works
  2. Save this file as export.ndjson and upload to your saved objects
  3. Click into the saved search we just uploaded, "Bedrock token tracking"
  4. Ensure you see events with tokens for bedrock
  5. Back to the Assistant, create a bedrock connector with bad credentials and ensure the Assistant returns an error when you attempt to use this connector

A note on api_alerting_integration tests

I spent 2 full work days trying to get these working. I’m trying to get the stream response from supertest. I can see the stream running on the backend, but when it comes back to supertest it only returns the readableState, not the stream itself. If there is anyone who can pair, I’d appreciate it. Otherwise, the code is covered in jest by x-pack/plugins/stack_connectors/server/connector_types/bedrock/bedrock.test.ts

@stephmilovic stephmilovic added release_note:enhancement WIP Work in progress Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore skip-ci v8.12.0 labels Nov 7, 2023
Comment on lines -75 to -77
// make sure the request is only triggered once,
// even with multiple subscribers
shareReplay(1),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shareReplay causes the observable to be hot, which means unsubscribe does not actually stop the stream

new Transform({
transform(chunk, encoding, callback) {
const encoder = new TextEncoder();
const decoder = new EventStreamCodec(toUtf8, fromUtf8);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find documentation on how to handle their codec. I did find awsdocs/aws-doc-sdk-examples#5541 for when documentation should be added.

I followed the bedrock extension in the langchainjs repo here: https://github.com/langchain-ai/langchainjs/pull/2367/files#diff-ead6be41360b3bf1c4559ea4d0069a015b69a57d5f8c51d0cce73253fde30aa2R246-R268

@stephmilovic stephmilovic removed WIP Work in progress skip-ci labels Nov 14, 2023
@stephmilovic stephmilovic marked this pull request as ready for review November 14, 2023 17:30
@stephmilovic stephmilovic requested review from a team as code owners November 14, 2023 17:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! LGTM. Thanks Steph

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM, left a comment about missing (trivial) jest test case

Regarding the functional test, can you use something other than supertest? I saw a thread somewhere of you asking about this, and I think someone else mentioned using node-fetch or such. If that can be made to work, no problem! If not, I guess there's something in our test framework that forces the use of supertest?

Worse case, create some kind of manual test in stack_connectors like this one we have in the actions plugin - so we have something to test with ... https://github.com/elastic/kibana/blob/main/x-pack/plugins/actions/server/manual_tests/forward_proxy.js

const passThrough = new PassThrough();

supertest
.post(`/internal/elastic_assistant/actions/connector/${bedrockActionId}/_execute`)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My test wasn't working because I was trying to call execute route directly, instead of my special route that handles the stream.. duh! Thanks @dgieselaar for helping me figure this out!

@stephmilovic stephmilovic requested a review from pmuellr November 15, 2023 20:30
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 12.8MB 12.8MB +754.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
stackConnectors 39.3KB 39.4KB +62.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stephmilovic stephmilovic merged commit d201610 into elastic:main Nov 16, 2023
27 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 16, 2023
@jamesspi jamesspi added this to the 8.12 milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.12.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

7 participants