Skip to content

[8.x] [Security Solution] [AI Assistant] Replace polynomial regular expression with constant time string manipulation (#209314)#210147

Merged
KDKHD merged 3 commits intoelastic:8.xfrom
KDKHD:backport/8.x/pr-209314
Feb 7, 2025
Merged

[8.x] [Security Solution] [AI Assistant] Replace polynomial regular expression with constant time string manipulation (#209314)#210147
KDKHD merged 3 commits intoelastic:8.xfrom
KDKHD:backport/8.x/pr-209314

Conversation

@KDKHD
Copy link
Copy Markdown
Member

@KDKHD KDKHD commented Feb 7, 2025

Backport

This will backport the following commits from main to 8.x:

Questions ?

Please refer to the Backport tool documentation

…ion with constant time string manipulation (#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)
@elasticmachine
Copy link
Copy Markdown
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

@KDKHD KDKHD merged commit d140b6d into elastic:8.x Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport This PR is a backport of another PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants