[Reporting] ReportingStore module #69426
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsullivan
force-pushed
the
reporting/store
branch
5 times, most recently
from
b552cfc
to
72a4540
Compare
tsullivan
force-pushed
the
reporting/store
branch
from
72a4540
to
82a8525
Compare
tsullivan
added
(Deprecated) Feature:Reporting
tsullivan
commented
Jun 24, 2020
| @@ -37,26 +36,25 @@ type GenericWorkerFn<JobParamsType> = ( | |||
| ...workerRestArgs: any[] | |||
| ) => void | Promise<TaskRunResult>; | |||
|
|
|||
| export async function createQueueFactory<JobParamsType, JobPayloadType>( | |||
There was a problem hiding this comment.
These generics were not used, so I removed them
There was a problem hiding this comment.
Pray I don't alter it further
| _primary_term: number; | ||
| } | ||
|
|
||
| export type Job = EventEmitter & { |
There was a problem hiding this comment.
This type is no longer needed: replaced with Report class
| }; | ||
| }; | ||
|
|
||
| export type EnqueueJobFn = <JobParamsType>( |
There was a problem hiding this comment.
Handling JobParamsType isn't necessary since jobParams is just pass-through
| @@ -40,21 +37,6 @@ export class Esqueue extends EventEmitter { | |||
| }); | |||
| } | |||
|
|
|||
| addJob(jobtype, payload, opts = {}) { | |||
There was a problem hiding this comment.
Replaced with ReportingStore.addJob
|
|
||
| const puid = new Puid(); | ||
|
|
||
| export class Job extends events.EventEmitter { |
There was a problem hiding this comment.
Replaced with Report class, minus the ES client stuff, which is part of ReportingStore
tsullivan
commented
Jun 24, 2020
| }) | ||
| .then(() => true) | ||
| .catch((err) => { | ||
| /* FIXME creating the index will fail if there were multiple jobs staged in parallel. |
There was a problem hiding this comment.
This comment is carried over as-is from the ESQueue x-pack/plugins/reporting/server/lib/esqueue/helpers/create_index.js
tsullivan
commented
Jun 24, 2020
| }); | ||
| } | ||
|
|
||
| private async saveReport(report: Report) { |
There was a problem hiding this comment.
This method is moved over from ESQueue's Job class constructor
tsullivan
force-pushed
the
reporting/store
branch
from
8e0fc11
to
29bf6a7
Compare
tsullivan
commented
Jun 24, 2020
| const isIndexExistsError = err.message.match(/resource_already_exists_exception/); | ||
| if (isIndexExistsError) { | ||
| // Do not fail a job if the job runner hits the race condition. | ||
| this.logger.warn(`Automatic index creation failed: index already exists: ${err}`); |
There was a problem hiding this comment.
This fixes a bug that was a FIXME comment: https://github.com/elastic/kibana/blob/2f67cbf/x-pack/plugins/reporting/server/lib/esqueue/helpers/create_index.js#L93
Continuing from index creation failure, when the index already exists, is now actually logged
tsullivan
commented
Jun 24, 2020
|
|
||
| import { LevelLogger } from '../lib'; | ||
|
|
||
| export function createMockLevelLogger() { |
There was a problem hiding this comment.
this would have been useful to have earlier :)
|
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
| const isPollingEnabled = config.get('queue', 'pollEnabled'); | ||
|
|
||
| const elasticsearch = await reporting.getElasticsearchService(); |
There was a problem hiding this comment.
Not seeing if this changed from sync to async here. I'm assuming so...
There was a problem hiding this comment.
This method was never async, so this is a little cleanup. It's something that doesn't get warned about.
💚 Build SucceededBuild metricspage load asset sizebeta
History
To update your PR or re-run it, just comment with: |
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
There was a problem hiding this comment.
This is starting to feel more like a "constant" to me since it's mostly static. Not sure on your thoughts there
There was a problem hiding this comment.
We may need to visit this more often than other constants. I think it's best to keep this easy to find.
|
|
||
| const esqueue = await createQueueFactory(reportingCore, logger); // starts polling for pending jobs | ||
| const enqueueJob = enqueueJobFactory(reportingCore, logger); // called from generation routes | ||
| const store = new ReportingStore(reportingCore, logger); |
There was a problem hiding this comment.
How do you feel about adding an instance of the store to the reporting-core object, and having it be access via a getter like we do with the other utilities?
There was a problem hiding this comment.
++.
We can shrink the options that get passed to pluginStart and have the core instance provide those things for itself, and provide getters.
There was a problem hiding this comment.
For now, my take on this ended up with more refactoring than I intended. Right now, the reporting store is to be set up in a synchronization with:
- having the config available, to pass the constructor
- being made available from ReportingCore before
create_queuegets called. It got harder to sync that when ESQueue gets the store from ReportingCore instead of receiving it as a parameter
A few PRs from now when ESQueue is gone, we can revisit this.
joelgriffith
suggested changes
Jun 25, 2020
joelgriffith
approved these changes
Jun 25, 2020
tsullivan
added a commit
to tsullivan/kibana
that referenced
this pull request
Jun 25, 2020
* Add store class * fix tests * fix the createIndex bug * add reportingstore test * change function args * nits * add test for automatic index creation failure recovery # Conflicts: # x-pack/plugins/reporting/server/lib/esqueue/job.js
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Jun 26, 2020
…t-pipelines/editor-error-messages * ingest-pipelines/editor-dropzone-refinement: (122 commits) Fixes for monaco XJSON grammar parser and update form copy Added cancel move button Refactor SCSS values to variables use classNames as it is intended to be used Remove box shadow on all nested tree items Rename variables and prefix booleans with "is" Fixes bug on color picker defaults on TSVB (elastic#69889) [DOCS] Fixes wording in Upload a CSV section (elastic#69969) [Discover] Validate timerange before submitting query to ES (elastic#69363) [Maps] avoid using MAP_SAVED_OBJECT_TYPE constant when defining URL paths (elastic#69723) [Maps] Fix icon palettes are not working (elastic#69937) [Ingest Manager] Fix typo in constant name (elastic#69919) [test] skip status.allowAnonymous tests on cloud (elastic#69017) Fix backport (elastic#70003) [Reporting] ReportingStore module (elastic#69426) Add reporting assets to the eslint ignore file (elastic#69968) [Discover] set minBarHeight for high cardinality data (elastic#69875) Add featureUsage API to licensing context provider (elastic#69838) Fix uncaught typecheck merge conflict (elastic#70001) Use TS to discourage SO mappings with dynamic: false / dynamic: true (elastic#69927) ...
kibanamachine
added
the
backport missing
|
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
1 similar comment
|
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
kibanamachine
removed
the
backport missing
tsullivan
added a commit
that referenced
this pull request
Jul 1, 2020
* [Reporting] ReportingStore module (#69426) * Add store class * fix tests * fix the createIndex bug * add reportingstore test * change function args * nits * add test for automatic index creation failure recovery # Conflicts: # x-pack/plugins/reporting/server/lib/esqueue/job.js * fix ts * fix bugs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Part of #69340
ESQueue is currently responsible for creating the time-based Reporting index, which is abstracted in its
addJobmethod. That functionality must be removed to allow ESQueue to be replaced with the Task Manager service.This PR adds a new class called ReportingStore, which has feature parity with the functionality previously held by Esqueue.addJob.
Checklist
Delete any items that are not applicable to this PR.
For maintainers