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

[Search] Add telemetry for data plugin search service #70677

Merged
merged 45 commits into from
Jul 15, 2020

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Jul 2, 2020

Summary

Relies on #69333.
Partially addresses #62964.

Adds telemetry for data plugin search service. It currently tracks the following events:

  • When a query times out
  • When a group of queries is explicitly cancelled by the user
  • When the popup for running queries beyond timeout is shown
  • When the popup for long running queries is dismissed
  • When a user runs a group of queries beyond timeout

To do

  • Add unit tests

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana Feature:Telemetry v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 2, 2020
@lukasolson lukasolson requested a review from lizozom July 2, 2020 23:12
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

LGTM

@lizozom lizozom requested a review from rudolf July 14, 2020 07:43
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
data 204 -1 205

miscellaneous assets size

id value diff baseline
data 397.3KB +2.2KB 395.2KB
dataEnhanced 78.4KB +171.0B 78.2KB
upgradeAssistant 22.5KB -24.0B 22.6KB
total - +2.3KB -

page load bundle size

id value diff baseline
data 1.4MB +8.7KB 1.4MB
dataEnhanced 177.7KB +420.0B 177.3KB
total - +9.1KB -

History

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

@lizozom lizozom merged commit 25d143f into elastic:master Jul 15, 2020
lizozom pushed a commit to lizozom/kibana that referenced this pull request Jul 15, 2020
* [search] Refactor the way search strategies are registered/retrieved on the server

* Fix types and tests and update docs

* Fix failing test

* Fix build of example plugin

* Fix functional test

* Make server strategies sync

* Move strategy name into options

* docs

* Remove FE strategies

* TypeScript of hell
delete search explorer

* Fix search interceptor OSS tests

* typos

* test cleanup

* Update search interceptor tests and abort utils

* [Search] Add telemetry for data plugin search service

* Add tracking of average query time

* Add tests and rename to collectors

* Fix TS

* Fixed interceptor jest tests

* Add to kibana json

* docs

* Properly use observables rather than only during setup

* Update or create

* Swallow version conflict errors

Co-authored-by: Liza K <liza.katz@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 15, 2020
* master:
  [APM] Add error rates to Service Map popovers (elastic#69520)
  [Security Solution][Detection Engine] - Update exceptions logic (elastic#71512)
  [Security Solution] Full screen timeline, Collapse event (elastic#71786)
  [Security Solution][Exception Modal] Create endpoint exception list if it doesn't already exist (elastic#71807)
  [Detection Rules] Add 7.9 rules (elastic#71808)
  [Search] Add telemetry for data plugin search service (elastic#70677)
  Add @elastic/safer-lodash-set as an alternative to lodash.set (elastic#67452)
  [tests] Temporarily skipped to promote snapshot
lizozom added a commit that referenced this pull request Jul 15, 2020
* [search] Refactor the way search strategies are registered/retrieved on the server

* Fix types and tests and update docs

* Fix failing test

* Fix build of example plugin

* Fix functional test

* Make server strategies sync

* Move strategy name into options

* docs

* Remove FE strategies

* TypeScript of hell
delete search explorer

* Fix search interceptor OSS tests

* typos

* test cleanup

* Update search interceptor tests and abort utils

* [Search] Add telemetry for data plugin search service

* Add tracking of average query time

* Add tests and rename to collectors

* Fix TS

* Fixed interceptor jest tests

* Add to kibana json

* docs

* Properly use observables rather than only during setup

* Update or create

* Swallow version conflict errors

Co-authored-by: Liza K <liza.katz@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 15, 2020
* master:
  [APM] Add error rates to Service Map popovers (elastic#69520)
  [Security Solution][Detection Engine] - Update exceptions logic (elastic#71512)
  [Security Solution] Full screen timeline, Collapse event (elastic#71786)
  [Security Solution][Exception Modal] Create endpoint exception list if it doesn't already exist (elastic#71807)
  [Detection Rules] Add 7.9 rules (elastic#71808)
  [Search] Add telemetry for data plugin search service (elastic#70677)
  Add @elastic/safer-lodash-set as an alternative to lodash.set (elastic#67452)
  [tests] Temporarily skipped to promote snapshot
@chrisronline
Copy link
Contributor

Hey folks,

Looking at this code, it seems the response will also include a total field, even though it does not show up in the interface. I think this is happening because we are seeing a structure like this in the monitoring parity tests:

"search": {
        "successCount": 0,
        "total": 0,
        "errorCount": 0,
        "averageDuration": null
      },

Does this seem right?

@TinaHeiligers
Copy link
Contributor

@lukasolson Are you planning to monitor anything via the telemetry service?

@lizozom
Copy link
Contributor

lizozom commented Jul 22, 2020

@chrisonline, is that a problem?
@TinaHeiligers yes, that's the intention of this PR. Why?

@TinaHeiligers
Copy link
Contributor

@lizozom I ask because I don't see an issue in the Telemetry repo asking to update the mapping for this issue. ui-metrics don't have a schema and we need issues requesting an update to the mapping with the shape of the data we are shipping from Kibana.

@chrisronline
Copy link
Contributor

@lizozom It's causing an issue with monitoring parity tests between legacy and metricbeat collection. I don't think it is related to the different collection methods, but rather an issue of the data being available sometimes and not always. Ideally, if the total field is returned and indexed when search-telemetry data is available, it should also be present when it's not (but default to 0).

@lizozom
Copy link
Contributor

lizozom commented Jul 27, 2020

@TinaHeiligers @lukasolson we'll open an issue for that.
@chrisronline not sure what we should be doing: Help us help you! :-)

@chrisronline
Copy link
Contributor

@lizozom Sorry, I did some more digging and I'm not sure this has anything to do with this code. I think it's more of a matter of keys colliding when merging telemetry data, which could be a bigger issue, but I'm still looking into it

});

return response.hits.hits.length
? (response.hits.hits[0]._source as Usage)
Copy link
Member

Choose a reason for hiding this comment

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

This as Usage cast is not true.
_source is returning something like:

         {
            "references": [],
            "updated_at": "2020-08-17T10:53:00.318Z",
            "search-telemetry": {
              "successCount": 2,
              "errorCount": 0,
              "averageDuration": 110
            },
            "type": "search-telemetry"
          }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Search Querying infrastructure in Kibana Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants