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

[Obs AI Assistant] Refactor ObservabilityAIAssistantClient #181255

Merged
merged 3 commits into from
Apr 30, 2024

Conversation

dgieselaar
Copy link
Member

@dgieselaar dgieselaar commented Apr 21, 2024

Refactors the Observability AI Assistant server-side client. Instead of using a mix of promises and Observables, we know use Observables where possible. This leads to more readable code, and makes things like error handling and logging easier.

This refactor purposely leaves the existing tests in place as much as possible. The functionality has however been broken into separate functions so we should be able to break up the existing tests into smaller pieces.

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@dgieselaar
Copy link
Member Author

/ci

@dgieselaar dgieselaar force-pushed the obs-ai-assistant-refactor-client branch from dbe3a89 to a496703 Compare April 21, 2024 10:08
@dgieselaar
Copy link
Member Author

/ci

@dgieselaar dgieselaar force-pushed the obs-ai-assistant-refactor-client branch 2 times, most recently from d005850 to 9a863cf Compare April 23, 2024 17:46
@dgieselaar
Copy link
Member Author

/ci

@dgieselaar
Copy link
Member Author

@elasticmachine merge upstream

@dgieselaar
Copy link
Member Author

/ci

@dgieselaar dgieselaar force-pushed the obs-ai-assistant-refactor-client branch from aa97a6a to ce4deca Compare April 24, 2024 09:59
@dgieselaar
Copy link
Member Author

/ci

@dgieselaar dgieselaar force-pushed the obs-ai-assistant-refactor-client branch from ce4deca to bb2d799 Compare April 25, 2024 09:03
@dgieselaar dgieselaar marked this pull request as ready for review April 25, 2024 09:04
@dgieselaar dgieselaar requested a review from a team as a code owner April 25, 2024 09:04
@dgieselaar dgieselaar added the release_note:skip Skip the PR/issue when compiling release notes label Apr 25, 2024
@dgieselaar
Copy link
Member Author

@elasticmachine merge upstream

Comment on lines 454 to 456
withAssistantSpan('get_connector', () =>
this.dependencies.actionsClient.get({ id: connectorId, throwIfSystemAction: true })
)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be using guardAgainstInvalidConnector? And should guardAgainstInvalidConnector be calling withAssistantSpan?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, will address

predefinedTitle || isConversationUpdate || !persist
? of(predefinedTitle || '').pipe(shareReplay())
: messagesWithUpdatedSystemMessage$.pipe(
switchMap((messages) =>
Copy link
Member

@sorenlouv sorenlouv Apr 29, 2024

Choose a reason for hiding this comment

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

Every time I see switchMap or forkJoin I stop to think what they do.
What's the best playground for getting familiar with rxjs? Codesandbox or is there something better suited for reactive programming?

Copy link
Member Author

@dgieselaar dgieselaar Apr 30, 2024

Choose a reason for hiding this comment

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

return new Observable<ChatCompletionChunkEvent | TokenCountEvent>((subscriber) => {
const shared = source$.pipe(shareReplay());
return (source$: Observable<ChatEvent>) => {
const shared$ = source$.pipe(shareReplay());
Copy link
Member

Choose a reason for hiding this comment

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

Q: is the fact that we use shareReplay everywhere cause for concern? This holds on to every emission for the lifetime of the request so can increase memory usage quite significantly. And if we don't properly end the observable we could end up with a memory leak.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question and something we should consider. In this case that's fine I think, as the Observable is short-lived, it doesn't keep on emitting values indefinitely.

@kibana-ci
Copy link
Collaborator

kibana-ci commented Apr 30, 2024

💚 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
observabilityAIAssistant 17.5KB 17.5KB +7.0B

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5874 +5874
total size - 6.7MB +6.7MB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
observabilityAIAssistant 24 25 +1

Page load bundle

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

id before after diff
observabilityAIAssistant 45.8KB 45.8KB +17.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
observabilityAIAssistant 5 6 +1

Total ESLint disabled count

id before after diff
observabilityAIAssistant 8 9 +1

History

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

@dgieselaar dgieselaar merged commit 6eba595 into elastic:main Apr 30, 2024
22 checks passed
@dgieselaar dgieselaar deleted the obs-ai-assistant-refactor-client branch April 30, 2024 12:14
@dgieselaar dgieselaar restored the obs-ai-assistant-refactor-client branch April 30, 2024 12:14
@dgieselaar dgieselaar deleted the obs-ai-assistant-refactor-client branch April 30, 2024 12:14
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.14 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.14:
- [Obs AI Assistant] Instructions & Claude improvements (#181058)

Manual backport

To create the backport manually run:

node scripts/backport --pr 181255

Questions ?

Please refer to the Backport tool documentation

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request May 1, 2024
…81255)

Refactors the Observability AI Assistant server-side client. Instead of
using a mix of promises and Observables, we know use Observables where
possible. This leads to more readable code, and makes things like error
handling and logging easier.

This refactor purposely leaves the existing tests in place as much as
possible. The functionality has however been broken into separate
functions so we should be able to break up the existing tests into
smaller pieces.

---------

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 6eba595)
@dgieselaar
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
8.14

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

Questions ?

Please refer to the Backport tool documentation

dgieselaar added a commit that referenced this pull request May 1, 2024
…81255) (#182237)

# Backport

This will backport the following commits from `main` to `8.14`:
- [[Obs AI Assistant] Refactor ObservabilityAIAssistantClient
(#181255)](#181255)

<!--- Backport version: 7.3.2 -->

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

<!--BACKPORT {commits} BACKPORT-->
yuliacech pushed a commit to yuliacech/kibana that referenced this pull request May 3, 2024
…81255)

Refactors the Observability AI Assistant server-side client. Instead of
using a mix of promises and Observables, we know use Observables where
possible. This leads to more readable code, and makes things like error
handling and logging easier.

This refactor purposely leaves the existing tests in place as much as
possible. The functionality has however been broken into separate
functions so we should be able to break up the existing tests into
smaller pieces.

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:Obs AI Assistant v8.14.0 v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants