[Remote clusters] guard against usageCollection plugin if unavailable#63284
Conversation
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
cjcenizal
left a comment
There was a problem hiding this comment.
Code LGTM, tested locally and everything works as expected, both when usageCollection is enabled and disabled. Just had one small suggestion.
| usageCollection = _usageCollection; | ||
| } | ||
|
|
||
| export function trackUiMetric(type: 'COUNT' | 'CLICK' | 'LOADED', name: string) { |
There was a problem hiding this comment.
How about we leverage the UiStatsMetricType here to ensure only acceptable values are passed in?
import { UiStatsMetricType } from '@kbn/analytics';
export function trackUiMetric(type: UiStatsMetricType, name: string) {
if (!usageCollection) {
return;
}
const { reportUiStats } = usageCollection;
reportUiStats(UIM_APP_NAME, type, name);
}This would require the callsites to provide the lower-case enum values, e.g.
trackUiMetric('count', eventName);In general I think I'm mostly suggesting this because I'm used to either seeing an enum key pulled directly off an enum (METRIC_TYPE.COUNT) or a reference to an enum value ('count'), but I'm not used to seeing a string reference to the enum's key ('COUNT').
EDIT: Another benefit is that it will automatically accommodate any new metric types that are added in the analytics package.
There was a problem hiding this comment.
👍 Great suggestion. I will update the PR.
There was a problem hiding this comment.
After looking into this some more, I don't believe your suggestion works, as UiStatsMetricType is actually a type that references the enum.
export type UiStatsMetricType = METRIC_TYPE.CLICK | METRIC_TYPE.LOADED | METRIC_TYPE.COUNT;
So, trackUiMetric('count', eventName); results in Argument of type '"count"' is not assignable to parameter of type 'UiStatsMetricType'.
I ended up importing METRIC_TYPE from @kbn/analytics. I think this might also be a little safer, as not all files have been converted to TS yet in remote clusters.
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* alerting/alert-services-mock: (107 commits) removed unused import added alert services mock and use it in siem [Metrics UI] Refactor With* containers to hooks (elastic#59503) [NP] Migrate logstash server side code to NP (elastic#63135) Clicking cancel in saved query save modal doesn't close it (elastic#62774) [Lens] Migration from 7.7 (elastic#62879) [Lens] Fix bug where suggestions didn't use filters (elastic#63293) Task/linux events (elastic#63400) [Remote clusters] guard against usageCollection plugin if unav… (elastic#63284) [Uptime] Remove pings graphql (elastic#59392) Index Pattern Field class - factor out copy_field code for future typescripting (elastic#63083) [EPM] add/remove package in package settings page (elastic#63389) Adjust API authorization logging (elastic#63350) Revert FTR: add chromium-based Edge browser support (elastic#61684) (elastic#63448) [Event Log] Adds namespace into save objects (elastic#62974) document code splitting for client code (elastic#62593) Escape single quotes surrounded by double quotes (elastic#63229) [Endpoint] Update cli mapping to match endpoint package (elastic#63372) update in-app links to metricbeat configuration docs (elastic#63295) investigation notes field (documentation / metadata) (elastic#63386) ...
I noticed while reviewing #63255 that we don't guard against the
usageCollectionplugin in remote clusters, even though it is defined as an optional plugin. This PR addresses that concern.To test, you can temporarily remove the
usageCollectionplugin here. Verify the app still works as expected.