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] Fix OpenAI token reporting #169156

Merged
merged 5 commits into from
Oct 18, 2023

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Oct 17, 2023

The OpenAI token reporting inadvertently got broken for security assistant in this PR.

When I implemented the invokeAI method, I returned the message as a string. However, I did not account for the fact that the action executor is looking for the usage object to report those metrics. Instead of just a string, invokeAI should return an object like { message: string; usage: UsageObject }.

@stephmilovic stephmilovic requested review from a team as code owners October 17, 2023 18:10
@stephmilovic stephmilovic added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.11.0 v8.12.0 v8.11.1 labels Oct 17, 2023
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@stephmilovic stephmilovic requested a review from a team as a code owner October 17, 2023 18:38
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Response Ops changes 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 13.0MB 13.0MB -255.0B

History

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

Comment on lines +45 to +52
// if not langchain, call execute action directly and return the response:
if (!request.body.assistantLangChain) {
const result = await executeAction({ actions, request, connectorId });
return response.ok({
body: result,
});
}

Copy link
Member

@spong spong Oct 17, 2023

Choose a reason for hiding this comment

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

Did assistantLangChain need to be pushed server side for this fix? @andrew-goldstein intentionally kept the split in logic between calling the actions framework, and the new langchain implementation at the client API layer for separation of concerns. There's no intent to continue supporting an actions 'pass through' once this new API is validated (at least AFAIK), so unless this is somehow required for the fix I'd be hesitant to push this logic into this route.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is going to be necessary for the upcoming streaming work, so all good! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, decided to move it new because I wanted to parse the response to continue returning a string from this endpoint, and I knew I'd be moving it for streaming anyways.

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Checked out, tested locally, and code reviewed. Seeing token counts for OpenAI connectors in the event log, and the langchain code paths appear to be functioning as expected -- LGTM! 👍

@stephmilovic stephmilovic enabled auto-merge (squash) October 18, 2023 16:47
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

@stephmilovic stephmilovic merged commit 8284398 into elastic:main Oct 18, 2023
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.11 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.11:
- [Security solution] More Elastic Assistant tests (#168146)

Manual backport

To create the backport manually run:

node scripts/backport --pr 169156

Questions ?

Please refer to the Backport tool documentation

@stephmilovic
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

stephmilovic added a commit to stephmilovic/kibana that referenced this pull request Oct 18, 2023
(cherry picked from commit 8284398)

# Conflicts:
#	x-pack/packages/kbn-elastic-assistant/impl/assistant/api.test.tsx
stephmilovic added a commit that referenced this pull request Oct 18, 2023
)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[Security solution] Fix OpenAI token reporting
(#169156)](#169156)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Steph
Milovic","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-10-18T18:06:03Z","message":"[Security
solution] Fix OpenAI token reporting
(#169156)","sha":"8284398023648e850a8ca038bce8be6cc85cc51f","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Threat
Hunting","Team:
SecuritySolution","v8.11.0","v8.12.0","v8.11.1"],"number":169156,"url":"https://github.com/elastic/kibana/pull/169156","mergeCommit":{"message":"[Security
solution] Fix OpenAI token reporting
(#169156)","sha":"8284398023648e850a8ca038bce8be6cc85cc51f"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/169156","number":169156,"mergeCommit":{"message":"[Security
solution] Fix OpenAI token reporting
(#169156)","sha":"8284398023648e850a8ca038bce8be6cc85cc51f"}}]}]
BACKPORT-->

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team v8.11.0 v8.11.1 v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants