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

feat: Search snippets and appmap events using sqlite #2123

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kgilpin
Copy link
Contributor

@kgilpin kgilpin commented Nov 23, 2024

In the old/existing algorithm, the search context collects events from AppMap data, then looks up the source code for the code objects represented in that event list. Then it supplements these search results with some text snippets. The ratio of event-based results to text-search-based results is 3 to 1:

https://github.com/getappmap/appmap-js/pull/2123/files#diff-b247cb5820889b6fdee4a942ce59747b9dd5285ee212a7b89a840470b41c600dL129

In this new implementation, events and code snippets are combined together in the same index, and search and ranked together.

This prevents the search results from being dominated by AppMap events, in the case that the AppMap data is not truly very relevant to the search.

@@ -0,0 +1,136 @@
import { ContextV2 } from '@appland/navie';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is essentially buildContext, but with a more (IMO) clear name, since the job of the function is not just to build the context, but to collect it from the available assets.

@@ -39,18 +39,6 @@ export function navieMetadataV1(): RpcHandler<
{
name: '@test',
description: 'Write tests for your code.',
referenceUrl: 'https://appmap.io/docs/navie-reference/navie-commands.html#test',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restore these changes before merge

@@ -1,83 +0,0 @@
import { EventEmitter } from 'stream';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Be sure to restore this.

@kgilpin kgilpin force-pushed the feat/update-appmap-file-search branch from 03cdf79 to 4545b12 Compare November 23, 2024 16:53
@kgilpin kgilpin force-pushed the feat/update-snippet-search branch from 394ec08 to 921fbdb Compare November 23, 2024 17:11
@kgilpin kgilpin force-pushed the feat/update-appmap-file-search branch from 4545b12 to 0967960 Compare November 23, 2024 21:40
Base automatically changed from feat/update-appmap-file-search to main December 1, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant