-
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
[Remote clusters] guard against usageCollection plugin if unavailable #63284
[Remote clusters] guard against usageCollection plugin if unavailable #63284
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Great suggestion. I will update the PR.
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.
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
usageCollection
plugin in remote clusters, even though it is defined as an optional plugin. This PR addresses that concern.To test, you can temporarily remove the
usageCollection
plugin here. Verify the app still works as expected.