Conversation
8d567b7 to
ed6c080
Compare
|
Pinging @elastic/kibana-platform (Team:Platform) |
ed6c080 to
960d8a3
Compare
src/core/server/saved_objects/migrations/kibana/kibana_migrator.mock.ts
Outdated
Show resolved
Hide resolved
| expect(statusUpdates.map(({ level, summary }) => ({ level, summary }))).toMatchInlineSnapshot(` | ||
| Array [ | ||
| Object { | ||
| "level": 2, |
There was a problem hiding this comment.
That demonstrates why we should use string enum https://www.typescriptlang.org/docs/handbook/enums.html#string-enums or a union of strings.
There was a problem hiding this comment.
Yeah I think the sorting property that I liked from a regular integer enum can be expressed as a utility. String readability is probably more important. 👍
There was a problem hiding this comment.
Actually on second thought, I think we're more often concerned with comparisons than we are with the string output (which is really only needed in the API outputs).
If we move to string enums, we would probably want to include a utility like this:
export const statusComparator = {
gt: (a: ServiceStatusLevel, b: ServiceStatusLevel): boolean => compareFn(a, b) === 1,
gte: (a: ServiceStatusLevel, b: ServiceStatusLevel): boolean => compareFn(a, b) >= 0,
lt: (a: ServiceStatusLevel, b: ServiceStatusLevel): boolean => compareFn(a, b) === -1,
lte: (a: ServiceStatusLevel, b: ServiceStatusLevel): boolean => compareFn(a, b) <= -1,
eq: (a: ServiceStatusLevel, b: ServiceStatusLevel): boolean => compareFn(a, b) === 0,
compareFn,
};However, I think this will be far less discoverable. Since doing comparisons like esStatus >= unavailable may be very common, I don't think this is an improvement. WDYT?
There was a problem hiding this comment.
However, I think this will be far less discoverable. Since doing comparisons like esStatus >= unavailable may be very common, I don't think this is an improvement. WDYT?
Optional: We can define each level as an object or a class instance with a valueOf method to make them comparable.
const available = {
toString() {
return 'available';
},
valueOf() {
return 0;
},
};
const degraded = {
toString() {
return 'degraded';
},
valueOf() {
return 1;
},
};
const levels = {
available,
degraded,
};
console.log(levels.available === levels.available);
console.log(levels.available > levels.degraded);
console.log(levels.available === levels.degraded);
console.log(levels.available < levels.degraded);but the current implementation is ok for me
There was a problem hiding this comment.
const available = {
toString() {
return 'available';
},
valueOf() {
return 0;
},
};Would love to have java enum class implementation in TS. that would really solves this kind of issues in a graceful way...
There was a problem hiding this comment.
Good idea, switched to this pattern. It doesn't work for array sorting, but I'm less concerned about that being a common use case.
|
Updated addressing all comments. The only thing that I think needs discussion is whether we should use integer enums or a string enum. I prefer the integer enum since it allows for easy comparison between status levels, which is very convenient. As I outlined here we could provide utilities to make this easier with string enums, however I worry that it won't be as easily discoverable. |
src/core/server/saved_objects/migrations/kibana/kibana_migrator.mock.ts
Outdated
Show resolved
Hide resolved
| if (this.migrationResult === undefined || rerun) { | ||
| this.status$.next({ status: 'running' }); | ||
| this.migrationResult = this.runMigrationsInternal(); | ||
| } |
There was a problem hiding this comment.
I'm mixed about the fact to re-emit running status in case of runMigrations({ rerun: true}). OTOH, this should only be used for FTR tests, so I guess that's alright.
rudolf
left a comment
There was a problem hiding this comment.
Happy for this to be merge if we've removed the double replay
| distinctUntilChanged(isDeepStrictEqual), | ||
| shareReplay(1) |
There was a problem hiding this comment.
We do another distinctUntilChanged and shareReplay in setupCoreStatus so we can remove this one.
There was a problem hiding this comment.
Maybe the shareReplay could be removed but I think we could still produce duplicate statuses since the summary rollup may or may not be the same value.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…chore/put-all-xjson-together * 'master' of github.com:elastic/kibana: (35 commits) [SIEM] [Detection Engine] Fixes bug when notification doesn't… (elastic#63013) [SIEM][Detection Engine] Fix rule notification critical bugs Add Error Exception Type Column (elastic#59596) [APM] Agent remote configuration: changes in Java property descriptions (elastic#62282) [Alerting] Displays warning when a permanent encryption key is missing and hides alerting UI appropriately (elastic#62772) FTR: add chromium-based Edge browser support (elastic#61684) [Ingest] Data source configuration validation UI (elastic#61180) restore empty_kibana after saved objects test (elastic#62951) Index pattern management plugin - src/legacy/core_plugins/management => new platform plugin (elastic#62594) Add basic StatusService (elastic#60335) [kbn/optimizer] link to kibanaReact/kibanaUtils plugins (elastic#62720) [APM] Service map - fixes layout issues for maps with no rum services (elastic#62887) Exclude disabled datasources and streams from agent config (elastic#62869) [Alerting] Fix validation support for nested IErrorObjects (elastic#62833) [Metrics UI] Invalidate non-count alerts which have no metrics (elastic#62837) Add --filter option to API docs script (elastic#62888) [Maps] fix attribution overflow with exit full screen button (elastic#62699) [Uptime]Alerting UI text in case filter is selected (elastic#62570) [Maps] Show create filter button for top-term tooltip property (elastic#62461) skip flaky suite (elastic#59030) ... # Conflicts: # src/plugins/es_ui_shared/public/index.ts
* master: (40 commits) [ML] Functional transform tests - stabilize source selection (elastic#63087) add embed flag to saved object url as well (elastic#62926) [SIEM] [Detection Engine] Fixes bug when notification doesn't… (elastic#63013) [SIEM][Detection Engine] Fix rule notification critical bugs Add Error Exception Type Column (elastic#59596) [APM] Agent remote configuration: changes in Java property descriptions (elastic#62282) [Alerting] Displays warning when a permanent encryption key is missing and hides alerting UI appropriately (elastic#62772) FTR: add chromium-based Edge browser support (elastic#61684) [Ingest] Data source configuration validation UI (elastic#61180) restore empty_kibana after saved objects test (elastic#62951) Index pattern management plugin - src/legacy/core_plugins/management => new platform plugin (elastic#62594) Add basic StatusService (elastic#60335) [kbn/optimizer] link to kibanaReact/kibanaUtils plugins (elastic#62720) [APM] Service map - fixes layout issues for maps with no rum services (elastic#62887) Exclude disabled datasources and streams from agent config (elastic#62869) [Alerting] Fix validation support for nested IErrorObjects (elastic#62833) [Metrics UI] Invalidate non-count alerts which have no metrics (elastic#62837) Add --filter option to API docs script (elastic#62888) [Maps] fix attribution overflow with exit full screen button (elastic#62699) [Uptime]Alerting UI text in case filter is selected (elastic#62570) ...
Summary
Related to #41983
Follows the spec outlined in RFC-0010.
This implements the basics of the StatusService + exposes the core status of the Elasticsearch and SavedObject services. These values are not currently being read by anything in the system yet, but will be in a future change.
Checklist
Delete any items that are not applicable to this PR.
For maintainers