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

Integrate new platform (core) server side into Kibana #18951

Merged
merged 182 commits into from Jul 11, 2018

Conversation

Projects
None yet
7 participants
@azasypkin
Copy link
Member

commented May 9, 2018

In this PR we're integrating base parts of the new platform (later core) server side into Kibana. At this point there are only two major touch points between new core and the rest of Kibana:

  • Now it's the core who listens on HTTP ports, handles TLS configuration and base path proxy. All requests to Kibana will hit HTTP server exposed by the core first and it will decide whether request can be solely handled by the new platform or request should be forwarded to the legacy Kibana. Technically we have two Hapi servers now (one for core and one for legacy Kibana), but only one underlying Node HTTP listener. This setup allows us to gradually introduce any "pre-route" processing code, expose new routes or replace old ones handled by the legacy platform currently.
  • Once config has been validated by the legacy Kibana, part of it will also be validated by the new core so that we can make config validation stricter with the new config validation system (even though it's based on Joi internally we introduced custom rules tailored to our needs like byteSize or duration). That means you can notice new config validation error messages if config value has been accepted by legacy config validation code, but rejected by the core.

With this PR legacy Kibana will still be started first using existing CLI and will bootstrap core only when needed, but we're going to inverse this in #19994.


New platform formerly resided in new-platform feature branch and then major part of it has been migrated to kbn-core feature branch. new-platform feature branch is going to stay for a while so that we can migrate the missing parts when we're ready. Here is a list of notable kbn-core vs new-platform differences:

  • Removed plugins supporting code, Elasticsearch client, cli
  • Reorganized platform (now core) directory structure according to #12466
  • Made kbn-observable package a part of the core and moved it to src/core/lib to use relative paths, removed unused operators
  • Transformed @kbn/utils into src/core/server/config/schema and based it on Joi internally

kimjoar and others added some commits Jun 20, 2017

Rename 'xpack_api_polling_frequency_millis' to 'api_polling_frequency' (
#12687)

* Rename 'xpack_api_polling_frequency_millis' to 'api_polling_frequency'

* prettier
Cleaner Pid observable code (#12702)
* Cleaner Pid observable code

* remove unused arg
Implement mapOf setting type (#12703)
* Implement mapOf setting type

* better variable names

* sharing code
@epixa

epixa approved these changes Jul 9, 2018

Copy link
Member

left a comment

This was reviewed over the last year in a long series of individual PRs. I just did a high level once over on the code to double check, and it looks good.

Please update the PR title and description though. To most people, this PR represents the first addition of the new platform. Can you update this PR to detail the changes?

LGTM

@archanid

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

Question about the kbn-observable that we changed into src/core/lib/kbn_observable: why didn't we move the share_last operator over? Should we make a note about this, in case we plan to reimplement that? I assume we don't want it anymore at all?

@archanid
Copy link
Contributor

left a comment

Can we either add some follow up about the stuff that we didn't pull over from kbn-observable, or include them here? If this gets merged as is, the things that didn't make it will be lost/forgotten.

@epixa

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

@archanid Are you referring to kbn-observable functionality from the new-platform branch that wasn't included here? If so, that shouldn't block this change.

The new-platform branch was abandoned the moment we created kbn-core, it continues to exist only so we can reference and potentially scavenge pieces from it in the future if we need to. So if there is a feature from it that you'd like to use going forward, you can copy it from that branch and PR to master just like any other change.

@archanid

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

@epixa yes, indeed.
Hm, but why didn't we include everything but that one operator? I just wonder. The description says we pulled the whole kbn-observable lib in here. Is it missing, or deliberately omitted?

@epixa

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

@archanid Our stance with code has always been to delete it if it's not being used, and it's not being used. I'm not sure if that's why it was omitted, but it's why I would have omitted it had I made the change :)

@azasypkin

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

Question about the kbn-observable that we changed into src/core/lib/kbn_observable: why didn't we move the share_last operator over? Should we make a note about this, in case we plan to reimplement that? I assume we don't want it anymore at all?

I just removed everything we don't need in the initial merge, and it was the only operator that depended on RxJS, but not used elsewhere. Since we have this in new-platform (as well as other pieces like elasticsearch client, plugins support etc.) we can pull it in when/if we need it.

@epixa

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

@archanid woo!

@azasypkin Please update the title/description and fix the dependency conflicts and get this beautiful beast merged!

@azasypkin azasypkin changed the title Merge `kbn-core` into `master` Integrate new platform (core) server side into Kibana Jul 11, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 11, 2018

@azasypkin azasypkin merged commit 1418310 into master Jul 11, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

azasypkin added a commit to azasypkin/kibana that referenced this pull request Jul 11, 2018

Integrate new platform (core) server side into Kibana (elastic#18951)
Co-authored-by: Kim Joar Bekkelund <kjbekkelund@gmail.com>
Co-authored-by: archana <archanid@users.noreply.github.com>
Co-authored-by: Spencer <spalger@users.noreply.github.com>
Co-authored-by: Court Ewing <court@epixa.com>
@azasypkin

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

6.x/6.4: f88d0b9

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 11, 2018

@azasypkin

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

Hmm, why CI is still running for this PR? :) Anyway it seems CI failed because of flaky test... Will be monitoring master today.

@w33ble

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

This PR broke Canvas. We just see a bunch of 404 errors in the console and nothing loads. There's an extra slash in the basepath now, not sure if that's related.

screenshot 2018-07-13 15 00 00


I opened #20802

@azasypkin azasypkin deleted the kbn-core branch Jul 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.