-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Grok debugger migration #60658
Grok debugger migration #60658
Conversation
Just noticed I forgot to add real validation for the |
Validation added. |
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 is looking great, I will run this later to make sure it works, and also let CJ and his team look it over as well.
@Kerry350 is there an easy way to test the different licensing scenarios to make sure they behave the same way as they used to? That part is confusing for me so I don't know if I'd be testing it correctly, maybe @cjcenizal can help there?
@jasonrhodes I'll test out some licensing scenarios. Thanks for highlighting that. According to our subscription matrix it's available in OSS, so it should be available in all licenses: |
Oh shoot, why does our subscription matrix claim that it's OSS? That's not right -- it's in X-Pack! I'll try to track this down. |
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.
Tested locally, code LGTM! I tested on a Basic and Trial license.
x-pack/plugins/grokdebugger/public/components/grok_debugger/grok_debugger.js
Outdated
Show resolved
Hide resolved
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.
@Kerry350 thanks a ton for taking on this work - it's looking great so far 🎉.
I left a suggestion and one change request. I also tested locally and am happy for this to be merged once @cjcenizal concerns are addressed and my comment regarding public plugin registration is addressed.
}); | ||
|
||
export function registerGrokSimulateRoute(framework) { | ||
framework.registerRoute( |
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.
I am not convinced we need to wrap both license state and the http service in a class. IRouter has quite a well designed API so my suggestion would be to create the router on the plugin and pass that in as a route dependency. Then we need to just wrap the route handler. For example see x-pack/plugins/index_management/server/services/license.ts
.
@cjcenizal @jloleysens Thanks both for the reviews 👍 I'll start addressing feedback now. |
@jloleysens I'd like to get your thoughts on keeping all registration synchronous, specifically I can't think how to provide the In ed846a1 I've changed things so that if the route is navigated to directly, and the license isn't active, the app still mounts but now displays an error callout (modelled off of search profiler's callout). But this doesn't handle disabling the tab. The following would work: async setup(coreSetup, plugins) {
registerFeature(plugins.home);
const license = await plugins.licensing.license$.pipe(first()).toPromise();
plugins.devTools.register({
order: 6,
title: i18n.translate('xpack.grokDebugger.displayName', {
defaultMessage: 'Grok Debugger',
}),
id: PLUGIN.ID,
enableRouting: false,
disabled: !license.isActive,
async mount(context, { element }) {
const [coreStart] = await coreSetup.getStartServices();
const license = await plugins.licensing.license$.pipe(first()).toPromise();
const { renderApp } = await import('./render_app');
return renderApp(license, element, coreStart);
},
});
} But that involves making things asynchronous again. Also (unless I'm missing something) the dev tools plugin doesn't have a way to update the status of the tabs / links after the fact, like with Do you have opinions on how the tab should be properly disabled if keeping things synchronous? |
…na helper wrapper bug
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.
Just left a couple small suggestions.
x-pack/plugins/grokdebugger/public/components/grok_debugger/grok_debugger.js
Show resolved
Hide resolved
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.
@Kerry350 I hear what you are saying regarding not being able to disable the tab. This is a limitation in how Dev Tools are being registered. The way SearchProfiler deals with it, historically, is to show the component but in a disabled state (as you have mimicked here). Dev Tools apps will need to be updated to become properly responsive to license changes in the future.
@flash1293 Would you mind weighing in here on whether registering Dev Tools will be disable-able in future (sorry if you're the wrong person to ping).
@jloleysens @cjcenizal Thanks again for the reviews. This should be good to go now once the build is green.
Cool, thanks for the clarification that there isn't anything that can be done here (yet) 👍 The only feedback I didn't action was the changing of the route registration handling. As this was stylistic vs something broken and there's only one route currently I've just left it for now. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Migrates Grok Debugger to new platform
🍾 Woohooo!! Thanks @Kerry350 ! |
* upstream/master: (69 commits) Adding PagerDuty icon to connectors cards (elastic#60805) Fix drag and drop flakiness (elastic#61993) Grok debugger migration (elastic#60658) Endpoint: Fix resolver SVG position issue (elastic#61886) [SIEM] version 7.7 rule import (elastic#61903) Added styles to make combobox list items wider for alerting flyout (elastic#61894) [UA] Tight worker loop can cause high CPU usage (elastic#60950) [ML] DF Analytics results table: use index pattern field format if one exists (elastic#61709) [ML] Catching unknown index pattern errors (elastic#61935) [Discover] Deangularize and euificate sidebar (elastic#47559) Endpoint: Add ts-node dev dependency (elastic#61884) Add an onBlur handler for the kuery bar. Only resubmit when input changes. (elastic#61901) [ML] Handle Empty Partition Field Values in Single Metric Viewer (elastic#61649) Auto interval on date histogram is getting displayed as timestamp per… (elastic#59171) [Maps] Explicitly pass fetch function to ems-client (elastic#61846) [SIEM][CASE] Fix aria-labels and translations (elastic#61670) [ML] Settings: Increase number of items that can be paged in calendars and filters lists (elastic#61842) [EPM] update epm filepath route (elastic#61910) APM] Set ignore_above to 1024 for telemetry saved object (elastic#61732) [Logs UI] Log stream row rendering (elastic#60773) ...
* master: (64 commits) Adding PagerDuty icon to connectors cards (elastic#60805) Fix drag and drop flakiness (elastic#61993) Grok debugger migration (elastic#60658) Endpoint: Fix resolver SVG position issue (elastic#61886) [SIEM] version 7.7 rule import (elastic#61903) Added styles to make combobox list items wider for alerting flyout (elastic#61894) [UA] Tight worker loop can cause high CPU usage (elastic#60950) [ML] DF Analytics results table: use index pattern field format if one exists (elastic#61709) [ML] Catching unknown index pattern errors (elastic#61935) [Discover] Deangularize and euificate sidebar (elastic#47559) Endpoint: Add ts-node dev dependency (elastic#61884) Add an onBlur handler for the kuery bar. Only resubmit when input changes. (elastic#61901) [ML] Handle Empty Partition Field Values in Single Metric Viewer (elastic#61649) Auto interval on date histogram is getting displayed as timestamp per… (elastic#59171) [Maps] Explicitly pass fetch function to ems-client (elastic#61846) [SIEM][CASE] Fix aria-labels and translations (elastic#61670) [ML] Settings: Increase number of items that can be paged in calendars and filters lists (elastic#61842) [EPM] update epm filepath route (elastic#61910) APM] Set ignore_above to 1024 for telemetry saved object (elastic#61732) [Logs UI] Log stream row rendering (elastic#60773) ...
Summary
This PR performs a full new platform migration of the Grok Debugger dev tool.
There isn't really anything special to mention - there aren't any temporary measures or "hacks" as this is a pretty small app and all needed APIs / features were available.
This blog post contains some examples that can be used for testing.