Skip to content
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 / Metrics] New Platform migration (full cutover) #54583

Merged
merged 162 commits into from
Feb 18, 2020

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Jan 13, 2020

Summary

This PR migrates our remaining ui/new_platform imports on the client side, and then performs the full cutover from x-pack/legacy/plugins to x-pack/plugins. Whilst a large portion of this PR is just file renames from moving directories, there have been some larger changes.

Note: There are some files showing as new files, rather than renames, under "Files changed". Everything was committed as a rename, but some anomalies seem to have snuck in. Things can be verified (as to whether it's actually new code) against the version in master within the legacy directory. Or feel free to ask me.

Notable changes

The two most notable changes, and the ones which have the most scope to have introduced subtle bugs or problems are:

  • Routing has switched from hash based routing to proper browser based routing, the same as Endpoint. Logs and Metrics also have two independent App Ids and App routes now. E.g. /app/metrics and /app/logs, there is no more /app/infra. A legacy redirect layer has been setup to redirect old app/infra# routes to their new locations. This can be removed in 8.0.0 (as a breaking change).

  • Logs and Metrics now have their own independent routers in preparation for the introduction of ScopedHistory to core (72641b2).

  • We are now using the Kibana version of lodash, whereas previously we were using our own lodash/fp module. This is, unfortunately, a really old (v3) version of lodash and some behaviour differs between the two modules. Parameter order is also different for some functions, and we lose the immutability / currying offered by lodash/fp. For the most part parameters have just been moved around, but some gross changes like ed6e5a7 have also been made (in that instance min and max used to return undefined for an empty array, not Infinity).

Less notable changes

These changes shouldn't have introduced any quirky behaviour, but are worth mentioning:

  • We've moved from the legacy Kibana URL utils to the QueryString module for our query string parsing / encoding. The only behaviour this should have changed is that ordering of params is different now, and therefore a few tests were changed to reflect this.

  • The Obervability telemetry hooks have moved from infra to the shared observability plugin (f5f41d7). (useTrackMetric, useTrackPageview etc). APM uses have also been changed.

  • Shared typings that existed in x-pack/legacy/common have been moved to the shared Observability plugin at x-pack/plugins/observability/public/typings. Right now they are duplicated in both places due to import restrictions, x-pack/legacy/common can be deleted when all plugins have fully migrated. (3e1547e).

Followups

We have tried to be pragmatic with the migration work, getting things working rather than perfect. These are things that should be followed up on down the line:

  • Right now a legacy version of the infra plugin still exists, this is only to facilitate savedObjectMappings whilst they're unavailable in the new platform. Once savedObjectMappings support is fully migrated to NP, we can delete the legacy folder in it's entirety.

  • legacy_singletons (d805286) have been added to allow certain portions of code to gain access to new platform features (similar to import xyz from 'ui/new_platform) where using the useKibana hook (for proper access to core and plugins) would have been hard / time consuming. These uses are all in logs code (none in metrics) and revolve around the core.http.fetch function. These uses should be refactored away over time. We should not add new uses of this going forward. Below is an example of why this was needed, and why refactoring certain areas would have taken longer:

legacy_singletons example

============== legacy_singletons example:

This example is the log entry rate results hook:

Here's the hook

It uses the useTrackedPromise hook - and if it were the internals of useTrackedPromise itself that managed the creation of the promise and the actual request initiation it would be fine, as we could just use useKibana inside of useTrackedPromise, but it's not.

We could also call useKibana at this useLogEntryRateResults level and pass it as a parameter, but that's not ideal. Instead, we have the createPromise option. In this instance it calls out to callGetLogEntryRateAPI.

This is callGetLogEntryRateAPI.

Just an async function, it's here that http.fetch is imported. We're not in any sort of context we can read from hooks here.

Metrics doesn't have this issue as the useHTTPRequest hook is used, which wraps useTrackedPromise, and thus access to useKibana is simple.

Moving forward logs can either migrate to also using the useHTTPRequest hook, or some other solution.

I do actually like the idea of options for an API call (the path, the body formatting etc) being in separate files. So another idea may be to keep these callGetLogEntryRateAPI style functions, but rather than being responsible for creating a promise and kicking off the request, they could just return an options object, that gets handed off to useHTTPRequest.

==============

  • In new platform world a (sensible) restriction is made that you can't just import any old code from inside of a plugins server and client folders, you may only import from the top level index (i.e. code that is in a contract to share with others). However, this rule also applies to our own common folder. For now // eslint-disable-next-line @kbn/eslint/no-restricted-paths rules have been added to all the locations where files in our common folder import from server or client. This isn't 100% ideal as theoretically another plugin could come along and import from here now, however we should be okay as our common folder is just common code for us. Moving forward we should refactor these uses - code that lives within common shouldn't be importing from client or server as it no longer makes it common. We should move towards this being for strictly common code (things like HTTP API types). As an example - we currently have React components living within common, these should definitely move to public and so on.

Testing

Unfortunately this is one of those cases where changes are far reaching and potentially could have broken anything. Any help with just navigating around, clicking things, filtering, opening sidemenus etc would be appreciated. Basically "just" using the apps 😅

For the testing of the legacy routing redirect layer below is a set of real old style URLs:

Old style URLs
  • /app/infra#

  • /app/infra#/home

  • /app/infra#/infrastructure

  • /app/infra#/infrastructure/inventory

  • /app/infra#/infrastructure/metrics-explorer?metricsExplorer=(chartOptions:(stack:!t,type:area,yAxisMode:fromZero),options:(aggregation:max,filterQuery:'',metrics:!((aggregation:max,color:color0,field:system.cpu.user.pct),(aggregation:max,color:color1,field:kubernetes.pod.cpu.usage.node.pct),(aggregation:max,color:color2,field:docker.cpu.total.pct))),timerange:(from:'2020-02-04T07:53:44.588Z',interval:%3E%3D10s,to:'2020-02-04T13:04:22.857Z'))

  • /app/infra#/infrastructure/settings

  • /app/infra#/logs

  • /app/infra#/logs/stream

  • /app/infra#/logs/stream?flyoutOptions=(flyoutId:!n,flyoutVisibility:hidden,surroundingLogsId:!n)&logPosition=(position:(tiebreaker:82069226,time:1580989944000),streamLive:!f)&logTextview=(textScale:large,wrap:!t)

  • /app/infra#/logs/log-rate?autoRefresh=(interval:30000,isPaused:!f)&timeRange=(endTime:now,startTime:now-2w)

  • /app/infra#/logs/log-categories?autoRefresh=(interval:60000,isPaused:!f)&timeRange=(endTime:now,startTime:now-2w)

  • /app/infra#/logs/settings

  • /app/infra#/infrastructure/metrics/host/gke-observability-8--observability-8--bc1afd95-cwh3?metricTime=(autoReload:!f,time:(from:'2020-02-06T10:51:04.103Z',interval:%3E%3D1m,to:'2020-02-06T11:51:04.103Z'))

  • /app/infra#/infrastructure/inventory?waffleFilter=(expression:'host.ip%20%3A%2010.8.2.249',kind:kuery)

  • /app/infra#/logs?logFilter=(expression:'host.ip%20%3A%2010.8.2.249',kind:kuery)

TODOs

Things that are needed before we can merge:

Known problems

These are known issues that we should merge around regardless:

Kerry350 and others added 20 commits January 13, 2020 12:56
- Introduces legacy singletons so that we can use core.http.fetch in the same fashion as kfetch
- Migrates to homePlugin.featureCatalogue.register for feature registration
- Accesses the data plugin for autocompletion via context
- Migrate saved objects client usage to context
- At this stage the only 'ui/' imports left are at the top level shim, prior to moving to the NP directory
…ules for public/server imports within common

/common code should be refactored in a followup. Common is not for other plugins to import from, just our own internal infra code.
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now completed a full review pass. The major outstanding problem is the breakage of the configuration schema. Aside from that most improvements can be handled in follow-up PRs with low risk.

I'm so grateful that you're handling this very complicated migration for us. 👏 This PR gives NP-hard and NP-complete new meanings.

@sgrodzicki sgrodzicki added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services and removed Team:infra-logs-ui labels Feb 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thanks again for all your hard work Kerry!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be some conflicts with our current PR we have up which moves APM server files out of legacy (#57532), but it looks like they will be fairly minimal, since the apm front-end is still in x-pack/legacy

@Kerry350 Kerry350 merged commit 0a6c748 into elastic:master Feb 18, 2020
@Kerry350
Copy link
Contributor Author

Thanks everyone for the reviews, help with implementation, answers to Platform questions etc. If there are any (tricky) conflicts that arise from this please reach out to me if I can help.

Tomorrow I'll start writing up the meta followup ticket to this, and will link all individual followup tickets within that so that nothing gets lost / forgotten.

It's been emotional NP ❤️

@Kerry350
Copy link
Contributor Author

All followups can now be found and tracked at #58003.

Kerry350 added a commit that referenced this pull request Feb 19, 2020
* Fully migrates metrics and logs to the NP

Co-authored-by: Jason Rhodes <[email protected]>
Co-authored-by: John Schulz <[email protected]>
Co-authored-by: Felix Stürmer <[email protected]>

Co-authored-by: Jason Rhodes <[email protected]>
Co-authored-by: John Schulz <[email protected]>
Co-authored-by: Felix Stürmer <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature Feature:Metrics UI Metrics UI feature Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.