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

[ES|QL] Query history #178302

Merged
merged 46 commits into from
Mar 28, 2024
Merged

[ES|QL] Query history #178302

merged 46 commits into from
Mar 28, 2024

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Mar 8, 2024

Summary

Closes #173217

Implements the query history component in the ESQL editor. The query history component displays the 20 most recent queries and it doesn't duplicate. If the user reruns a query it will update an existing one and not create a new entry.

image image

Important notes

Right now, the query history component has been implemented at:

  • Unified search ES|QL editor
  • Lens inline editing component
  • Alerts
  • Maps

I have hid it from ML data visualizer because it was very difficult to implement it there. There is a quite complex logic fetching the fields statistics so it was a bit complicated to add it there. ML team can follow up as they know the logic already and would be easier for them to adjust.

Flajy test runner

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5553

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula changed the title Esql query history [ES|QL] Query history Mar 8, 2024
@@ -152,7 +155,6 @@ export interface QueryBarTopRowProps<QT extends Query | AggregateQuery = Query>
dataViewPickerComponentProps?: DataViewPickerProps;
textBasedLanguageModeErrors?: Error[];
textBasedLanguageModeWarning?: string;
hideTextBasedRunQueryLabel?: boolean;
Copy link
Contributor Author

@stratoula stratoula Mar 20, 2024

Choose a reason for hiding this comment

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

ℹ️ Cleaning this up, I don't see this property being used anywhere in kibana and with this PR we are also not displaying the text in unified search.

@stratoula
Copy link
Contributor Author

/ci

@stratoula
Copy link
Contributor Author

/ci

@stratoula stratoula added release_note:feature Makes this part of the condensed release notes v8.14.0 backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana labels Mar 26, 2024
@stratoula stratoula marked this pull request as ready for review March 26, 2024 15:14
@stratoula stratoula requested review from a team as code owners March 26, 2024 15:14
@stratoula stratoula requested a review from a team March 26, 2024 16:28
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Code-only review. New Discover functional tests LGTM 👍

I'll leave a more thorough review of the ES|QL changes to our new teammate and resident @elastic/kibana-esql member, @drewdaemon 🙂

Copy link
Contributor

@doakalexi doakalexi left a comment

Choose a reason for hiding this comment

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

I see the query history when creating a rule from discover, but I do not see it when creating a rule from stack management. Is this expected, maybe I am not testing it correctly

@doakalexi
Copy link
Contributor

I see the query history when creating a rule from discover, but I do not see it when creating a rule from stack management. Is this expected, maybe I am not testing it correctly

I am sorry, it was testing problem on my end. I see it working correctly.

Copy link
Contributor

@doakalexi doakalexi left a comment

Choose a reason for hiding this comment

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

ResponseOps changes LGTM! Sorry for the noise

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

I love this feature!

packages/kbn-text-based-editor/src/editor_footer.tsx Outdated Show resolved Hide resolved
packages/kbn-text-based-editor/src/query_history.test.tsx Outdated Show resolved Hide resolved
const [editorMessages, setEditorMessages] = useState<{
errors: MonacoMessage[];
warnings: MonacoMessage[];
}>({
errors: serverErrors ? parseErrors(serverErrors, code) : [],
warnings: serverWarning ? parseWarning(serverWarning) : [],
});
// contains only client side validation messages
const [clientParserMessages, setClientParserMessages] = useState<{
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Context complicates things 👍

test/functional/services/esql.ts Outdated Show resolved Hide resolved
public async getHistoryItems(): Promise<string[][]> {
const queryHistory = await this.testSubjects.find('TextBasedLangEditor-queryHistory');
const tableBody = await this.retry.try(async () => queryHistory.findByTagName('tbody'));
const $ = await tableBody.parseDomContent();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I didn't know about this feature! I'm sure it's much more performant to load everything in one go.

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

LGTM! 🥳

@stratoula
Copy link
Contributor Author

/ci

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 1375 1378 +3
securitySolution 5038 5041 +3
stackAlerts 140 143 +3
textBasedLanguages 55 58 +3
total +12

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dataVisualizer 652.5KB 652.5KB +20.0B
lens 1.4MB 1.4MB +78.0B
stackAlerts 82.3KB 82.4KB +60.0B
textBasedLanguages 140.7KB 151.6KB +10.9KB
unifiedSearch 225.5KB 225.6KB +11.0B
total +11.0KB

Page load bundle

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

id before after diff
textBasedLanguages 5.5KB 5.6KB +108.0B
Unknown metric groups

API count

id before after diff
@kbn/text-based-editor 31 32 +1
textBasedLanguages 27 28 +1
total +2

ESLint disabled line counts

id before after diff
@kbn/text-based-editor 2 3 +1

Total ESLint disabled count

id before after diff
@kbn/text-based-editor 2 3 +1

History

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

@qn895
Copy link
Member

qn895 commented Mar 28, 2024

Tested and LGTM 🎉

Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Vis team changes LGTM! 🎉

@stratoula stratoula merged commit 884b94e into elastic:main Mar 28, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana release_note:feature Makes this part of the condensed release notes v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ES|QL] Query History
8 participants