-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Logs UI] Generalize ML module management #50662
[Logs UI] Generalize ML module management #50662
Conversation
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
💔 Build Failed
|
4d0243d
to
3344f32
Compare
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded
|
df3bc37
to
4b4268f
Compare
💔 Build Failed
|
💚 Build Succeeded
|
💚 Build Succeeded
|
1b4de9c
to
adb3ea2
Compare
💚 Build Succeeded |
💔 Build Failed |
jenkins, test this again |
💚 Build Succeeded |
@elasticmachine update branch |
💚 Build Succeeded |
Looks unrelated:
|
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍 This cleaned things up a lot, even with just the one use of generalised parts currently. The new directory structures look good too. I have left a couple of really minor comments.
...accumulatedJobStatus, | ||
[jobType]: 'unknown', | ||
}), | ||
{} as Record<JobType, JobStatus> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick: Could we use a generic type argument over the casting? (jobTypes.reduce<Record<JobType, JobStatus>>
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I usually prefer too, but in this case it doesn't work because the empty default object is not assignable to that record type:
Argument of type '{}' is not assignable to parameter of type 'Record<JobType, JobStatus>'.
...accumulatedJobStatus, | ||
[jobType]: 'initializing', | ||
}), | ||
{} as Record<JobType, JobStatus> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, generic type argument over casting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above 😉
|
||
import { useSourceContext } from '../../../containers/source'; | ||
import { useKibanaSpaceId } from '../../../utils/use_kibana_space_id'; | ||
// import { LogEntryRateJobsProvider } from './use_log_entry_rate_jobs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be deleted?
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
ℹ️ backport to 7.x is waiting for #52613 |
This abstracts the specific job details out of the ML module management hooks to enable re-use with the upcoming categorization module. closes elastic#50322
…le-sql-highlighting * 'master' of github.com:elastic/kibana: (56 commits) Migrate url shortener service (elastic#50896) Re-enable datemath in from/to canvas timelion args (elastic#52159) [Logs + Metrics UI] Remove eslint exceptions (elastic#50979) [Logs + Metrics UI] Add missing headers in Logs & metrics (elastic#52405) [ML] API integration tests - initial tests for bucket span estimator (elastic#52636) [Watcher] New Platform (NP) Migration (elastic#50908) Decouple Authorization subsystem from Legacy API. (elastic#52638) [APM] Fix some warnings logged in APM tests (elastic#52487) [ui/public/utils] Delete unused base_object & find_by_param (elastic#52500) [ui/public/utils] Move items into ui/vis (elastic#52615) fix newlines in kbn-analytics build script Add top level examples folder and command to run, `--run-examples`. (elastic#52027) feat(NA): add trap for SIGINT in the git precommit hook (elastic#52662) [DOCS] Updtes description of elasticsearch.requestHeadersWhitelist (elastic#52675) [Telemetry/Pulse] Updates advanced settings text for usage data (elastic#52657) [SIEM][Detection Engine] Adds the default name space to the end of the signals index [Logs UI] Generalize ML module management (elastic#50662) Removing stateful saved object finder (elastic#52166) Shim oss telemetry (elastic#51168) [Reporting/Screenshots] Do not fail the report if request is aborted (elastic#52344) ... # Conflicts: # src/legacy/core_plugins/console/public/legacy.ts # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/elasticsearch_sql_highlight_rules.ts # src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/components/full_request_component.ts # src/legacy/core_plugins/console/public/quarantined/src/sense_editor/row_parser.js
This abstracts the specific job details out of the ML module management hooks to enable re-use with the upcoming categorization module. closes elastic#50322
Summary
This abstracts the specific job details out of the ML module management hooks to enable re-use with the upcoming categorization module.
closes #50322
Implementation notes
I tried to apply the following methodology:
containers/log_analysis
have been made agnostic to the specific ML modulepages/logs/log_entry_rate
moduleDescriptor
data structure (i.e.logEntryRateModuleDescriptor
)ModuleSourceConfiguration
pages/logs/log_entry_rate
(mostly a rename frompages/logs/log_analysis
)components/logging
The resulting relationships between the resulting components and hooks on the coarse scale could be described with the following graph:
Checklist
[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibility