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] Search Sessions Monitoring Task #85253

Merged
merged 97 commits into from
Jan 11, 2021

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Dec 8, 2020

Summary

Contains #85253 (will be rebased on top)

This PR implements the Search Sessions monitoring task, as specified in the Search Sessions (formerly known as Background Sessions RFC).

The Search Session service is responsible for tracking sessions in memory and storing them, if the user chooses to do so, but after sessions are saved, the responsibility for tracking them is passed down to the monitoring task.

This task is responsible for checking up on in_progress search sessions, fetching the status of each search request and updating the search session's status (so it's correctly reflected in the Management UI for example).

Once available, this task would probably also be integrated with the push notification service, to provide more immediate completion notifications to the user.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Liza K and others added 30 commits November 24, 2020 16:51
Save IDs only in monitoring loop
@lizozom lizozom requested review from Dosant and lukasolson and removed request for a team January 7, 2021 16:07
@Dosant
Copy link
Contributor

Dosant commented Jan 7, 2021

Code lgtm, tested following things:

  1. Search discover, get results, then save session. Checked in devtools that search and session are in completed state 👍
  2. My other test didn't go well, once it passed and other time it failed 😢:

I took a sample dashboard and added a single slow viz with 60s shard delay agg. I expected to see in dev tools session completed in around 60s and it was the case once.
But other time long search went to error state with following error:

server    log   [18:11:10.937] [debug][dataEnhanced][data_enhanced][plugins] Found 1 running sessions
server    log   [18:11:10.938] [error][dataEnhanced][data_enhanced][plugins] ResponseError: resource_not_found_exception
    at onBody (/Users/antondosov/Dev/kibana/node_modules/@elastic/elasticsearch/lib/Transport.js:333:23)
    at IncomingMessage.onEnd (/Users/antondosov/Dev/kibana/node_modules/@elastic/elasticsearch/lib/Transport.js:260:11)
    at IncomingMessage.emit (events.js:327:22)
    at endReadableNT (internal/streams/readable.js:1327:12)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  meta: {
    body: { error: [Object], status: 404 },
    statusCode: 404,
    headers: {
      'content-type': 'application/json;charset=utf-8',
      'content-length': '285'
    },
    meta: {
      context: null,
      request: [Object],
      name: 'elasticsearch-js',
      connection: [Object],
      attempts: 0,
      aborted: false
    }
  }
}

Session also errored out as the result.
Not sure what went wrong. Maybe there is a race condition somewhere?

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Tested & functionality seems to be working as expected.

@LeeDr
Copy link
Contributor

LeeDr commented Jan 7, 2021

This seems like additional features targeted after 7.11.0 FF? 28 files changed (I didn't look but I'm sure some are tests) seems like a change we wouldn't put in after feature freeze.

@lizozom lizozom added v7.12.0 and removed v7.11.0 labels Jan 11, 2021
@lizozom
Copy link
Contributor Author

lizozom commented Jan 11, 2021

@Dosant what you saw is the fact that the extend api changes are still not merged.
So the session got expired while executing :)
Nice test!

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Didn't re-test since the last review considering #85253 (comment) is expected

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.

Other than a couple of naming/typing notes, the Alerting & TaskManager changes look good to me 👍

Thanks for doing this 🙏

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@lizozom lizozom merged commit 3eeec0f into elastic:master Jan 11, 2021
lizozom added a commit to lizozom/kibana that referenced this pull request Jan 11, 2021
* Monitor ids

* import fix

* solve circular dep

* eslint

* mock circular dep

* max retries test

* mock circular dep

* test

* jest <(-:C

* jestttttt

* [data.search] Move search method inside session service and add tests

* merge

* Move background session service to data_enhanced plugin

* Better logs
Save IDs only in monitoring loop

* Fix types

* Space aware session service

* ts

* initial

* initial

* Fix session service saving

* merge fix

* stable stringify

* INMEM_MAX_SESSIONS

* INMEM_MAX_SESSIONS

* use the status API

* Move task scheduling behind a feature flag

* Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts

Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

* Add unit tests

* Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts

Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

* Use setTimeout to schedule monitoring steps

* Update request_utils.ts

* settimeout

* tiny cleanup

* Core review + use client.asyncSearch.status

* update ts

* fix unit test

* code review fixes

* Save individual search errors on SO

* Don't re-fetch completed or errored searches

* Rename Background Sessions to Search Sessions (with a send to background action)

* doc

* doc

* jest fun

* rename rfc

* translations

* merge fix

* merge fix

* code review

* update so name in features

* Move deleteTaskIfItExists to task manager

* task_manager to ts project

* Move deleteTaskIfItExists to public contract

* mock

* use task store

* ts

* code review

* code review + jest

* Alerting code review

Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Anton Dosov <dosantappdev@gmail.com>
Co-authored-by: restrry <restrry@gmail.com>
lizozom added a commit that referenced this pull request Jan 11, 2021
* Monitor ids

* import fix

* solve circular dep

* eslint

* mock circular dep

* max retries test

* mock circular dep

* test

* jest <(-:C

* jestttttt

* [data.search] Move search method inside session service and add tests

* merge

* Move background session service to data_enhanced plugin

* Better logs
Save IDs only in monitoring loop

* Fix types

* Space aware session service

* ts

* initial

* initial

* Fix session service saving

* merge fix

* stable stringify

* INMEM_MAX_SESSIONS

* INMEM_MAX_SESSIONS

* use the status API

* Move task scheduling behind a feature flag

* Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts

Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

* Add unit tests

* Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts

Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

* Use setTimeout to schedule monitoring steps

* Update request_utils.ts

* settimeout

* tiny cleanup

* Core review + use client.asyncSearch.status

* update ts

* fix unit test

* code review fixes

* Save individual search errors on SO

* Don't re-fetch completed or errored searches

* Rename Background Sessions to Search Sessions (with a send to background action)

* doc

* doc

* jest fun

* rename rfc

* translations

* merge fix

* merge fix

* code review

* update so name in features

* Move deleteTaskIfItExists to task manager

* task_manager to ts project

* Move deleteTaskIfItExists to public contract

* mock

* use task store

* ts

* code review

* code review + jest

* Alerting code review

Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Anton Dosov <dosantappdev@gmail.com>
Co-authored-by: restrry <restrry@gmail.com>

Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Anton Dosov <dosantappdev@gmail.com>
Co-authored-by: restrry <restrry@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.