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.

Sign up for GitHub

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

Jump to bottom

Removed Alerting & Event Log deprecated fields that should not be using #85652

Merged
merged 3 commits into from
Dec 15, 2020
Merged

Removed Alerting & Event Log deprecated fields that should not be using #85652

merged 3 commits into from
Dec 15, 2020

Conversation

YulNaumenko
Copy link
Contributor

  1. Removed rest_total_hits_as_int from alerts and actions telemetry cluster request.
  2. Replaced rest_total_hits_as_int with the new proper query option track_total_hits for Event Log plugin.

@YulNaumenko YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 Feature:EventLog labels Dec 10, 2020
@YulNaumenko YulNaumenko self-assigned this Dec 10, 2020
@YulNaumenko YulNaumenko requested a review from a team as a code owner December 10, 2020 21:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@YulNaumenko YulNaumenko linked an issue Dec 10, 2020 that may be closed by this pull request
Alerting & Event Log are using deprecated fields we shouldn't be using #84222
Closed
gmmorris
gmmorris approved these changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM,

My only request is that we replace the casting to unknown as it breaks the typing and is unsafe.

Thanks! :D

x-pack/plugins/event_log/server/es/cluster_client_adapter.ts Outdated
@@ -286,15 +286,13 @@ export class ClusterClientAdapter {
hits: { hits, total },
}: SearchResponse<unknown> = await this.callEs('search', {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use the ESSearchResponse type you won't need to cast to unknown.

At the top add:

import { ESSearchResponse } from '../../../../typings/elasticsearch';

Then you can replace this with:

Suggested change
}: SearchResponse<unknown> = await this.callEs('search', {
}: ESSearchResponse<unknown, {}> = await this.callEs('search', {

Then below, replace the cast with:

        total: total.value,

pmuellr
pmuellr approved these changes Dec 14, 2020
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

I'm curious if/how we do function tests on these. Looked in x-pack/test/alerting_api_integration, and didn't see anything, but maybe I didn't know what to search for, or maybe it's somewhere else. Or we just don't have any.

@YulNaumenko
Copy link
Contributor Author

LGTM

I'm curious if/how we do function tests on these. Looked in x-pack/test/alerting_api_integration, and didn't see anything, but maybe I didn't know what to search for, or maybe it's somewhere else. Or we just don't have any.

I don't see any existing. How do you think we should test it? Just mock ESSearchResponse?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47130 47890 +760

History

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

@YulNaumenko YulNaumenko merged commit b7c9d22 into elastic:master Dec 15, 2020
@YulNaumenko YulNaumenko mentioned this pull request Dec 15, 2020
Merged
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Dec 15, 2020
@YulNaumenko
Removed Alerting & Event Log deprecated fields that should not be usi…
75f47b4 
…ng (elastic#85652)

* Removed Alerting & Event Log deprecated fields that should not be using

* fixed due to comments
@pmuellr
Copy link
Member

pmuellr commented Dec 15, 2020

LGTM
I'm curious if/how we do function tests on these. Looked in x-pack/test/alerting_api_integration, and didn't see anything, but maybe I didn't know what to search for, or maybe it's somewhere else. Or we just don't have any.

I don't see any existing. How do you think we should test it? Just mock ESSearchResponse?

I was thinking more about an E2E test, as FTS/FTR tests. I don't think unit tests with mocks is something worth the effort. And I'm not sure how this would work as FTS/FTR tests. Just curious ... :-)

gmmorris added a commit to ymao1/kibana that referenced this pull request Dec 15, 2020
@gmmorris
Merge branch 'master' into pr/85462
fa70d31 
* master: (66 commits)
  [Alerting] fixes broken Alerting Example plugin (elastic#85774)
  [APM] Service overview instances table (elastic#85770)
  [Security Solution] Unskip timeline creation Cypress test (elastic#85871)
  properly recognize enterprise licenses (elastic#85849)
  [SecuritySolution][Detections] Adds SavedObject persistence to Signals Migrations (elastic#85690)
  [TSVB] Fix functional tests flakiness and unskip them (elastic#85388)
  [Fleet] Change permissions for Fleet enroll role (elastic#85802)
  Gauge visualization can no longer be clicked to filter on values since Kibana 7.10.0 (elastic#84768)
  [Security Solution][Detections] Add alert source to detection rule action context (elastic#85488)
  [Discover] Don't display hide/show button for histogram when there's no time filter (elastic#85424)
  skip flaky suite (elastic#78553)
  License checks for alerts plugin (elastic#85649)
  skip flaky suite (elastic#84992)
  skip 'query return results valid for scripted field' elastic#78553
  Allow action types to perform their own mustache variable escaping in parameter templates (elastic#83919)
  [ML] More machine learning links in doc_links_service.ts (elastic#85365)
  Removed Alerting & Event Log deprecated fields that should not be using (elastic#85652)
  Closes elastic#79995 by adding new tab in transaction details to show related trace logs. (elastic#85859)
  Fix outdated jest snapshot
  [Maps] Surface on prem EMS (elastic#85729)
  ...
YulNaumenko added a commit that referenced this pull request Dec 15, 2020
@YulNaumenko
Removed Alerting & Event Log deprecated fields that should not be usi…
478aea0 
…ng (#85652) (#85897)

* Removed Alerting & Event Log deprecated fields that should not be using

* fixed due to comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmuellr pmuellr pmuellr approved these changes

@gmmorris gmmorris gmmorris approved these changes

Assignees

@YulNaumenko YulNaumenko

Labels
Feature:Actions Feature:Alerting Feature:EventLog release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Alerting & Event Log are using deprecated fields we shouldn't be using
5 participants
@YulNaumenko @elasticmachine @kibanamachine @pmuellr @gmmorris