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

[8.18] [Security Solution] [AI Assistant] Replace polynomial regular expression with constant time string manipulation (#209314) #210145

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

kibanamachine
Copy link
Contributor

Backport

This will backport the following commits from main to 8.18:

Questions ?

Please refer to the Backport tool documentation

…ion with constant time string manipulation (elastic#209314)

## Summary

Fixes elastic/kibana-team#1442

This PR replaces a poorly performing regular expression with a constant
time string manipulation approach.

Context:
The regex is used to remove all references from a string when a user
copies a message from the assistant and when conversation history is
passed to the LLM e.g.
```
"The sky is blue{reference(1234)} and the grass is green{reference(4321)}" -> "The sky is blue and the grass is green"
```

Changes:
- Replace the regular expression inside of `removeContentReferences()`
- Add tests to verify new logic is correct.
- Fix a bug in the contentReference markdown parser that was found by
@andrew-goldstein
[here](https://github.com/elastic/kibana/pull/209314/files#r1943198510)
- For alerts page citations, add a filter for open and acknowledge
alerts within the last 24 hours
[here](https://github.com/elastic/kibana/pull/209314/files#diff-f17fbe7edfe72943fecbe5ddd8dca6c024a48fe4f90bf4f66650cef16091b769R36)

### How to test new regex:
One of the changes in this PR improves the performance of a regex. In
real life, no one has ever reached any performance issues with this
regex's and I don't think it is realistically possible to reach that
limit without other things breaking (i.e. the message sent to/returned
by the assistant would need to be so large that it would exceed the
context window). Therefore, all we can test is that the functionality
still works as expected after this change.
- Enable the feature flag
```yaml
# kibana.dev.yml
xpack.securitySolution.enableExperimental: ['contentReferencesEnabled']
```
- Open the security assistant
- Ask the assistant a question about your alerts or a document in your
KB. The assistant response should contain citations.
- Copy the response to the clipboard using the copy button.
<img width="785" alt="image"
src="https://github.com/user-attachments/assets/edded3a3-8cb9-40a8-918e-a9718e7afc22"
/>
- Your clipboard should contain the response without any citations

### How to test the alerts page filter
- Ask a question about your open alerts and make sure a citation is
returned.
- Click on the citation
- Verify a new tab is opened and the alerts page is visible with a
filter for open and acknowledge alerts and there is a now-24h time
window filter.

### Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

- [X] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [X]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [X] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [X] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [X] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [X] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [X] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit 2bf8a24)
@kibanamachine kibanamachine merged commit 5b7bc06 into elastic:8.18 Feb 7, 2025
8 checks passed
@elasticmachine
Copy link
Contributor

💚 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 18.8MB 18.8MB +267.0B

History

cc @KDKHD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants