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.

Sign up for GitHub

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

Jump to bottom

Migrate kql telemetry and scripts api #52651

Merged
merged 9 commits into from
Dec 20, 2019
Merged

Migrate kql telemetry and scripts api #52651

merged 9 commits into from
Dec 20, 2019

Conversation

flash1293
Copy link
Contributor

This PR migrates the apis for kql telemetry (opting in/out) and the scripts api returning currently active scripting languages for scripted fields to the new platform.

As they are related to the services the data plugin already owns, these services are put into the data plugin as well

@flash1293 flash1293 added Feature:NP Migration v7.6.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 10, 2019
@flash1293 flash1293 marked this pull request as ready for review December 11, 2019 16:17
@flash1293 flash1293 requested a review from a team as a code owner December 11, 2019 16:17
@flash1293 flash1293 added Team:AppArch Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Dec 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@flash1293 flash1293 requested a review from lukeelmers December 11, 2019 16:18
@flash1293
Copy link
Contributor Author

Jenkins, test this

lukeelmers
lukeelmers approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM; thank you for doing this! (Tested Chrome OSX and verified routes are still working)

src/legacy/core_plugins/kibana/ui_setting_defaults.js
@@ -20,6 +20,7 @@
import moment from 'moment-timezone';
import numeralLanguages from '@elastic/numeral/languages';
import { i18n } from '@kbn/i18n';
import { DEFAULT_QUERY_LANGUAGE } from '../../../plugins/data/common';
Copy link
Member

Choose a reason for hiding this comment

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

We should import from public here if possible, instead of common

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is used on the server, so importing from public won't work. Funny enough as it's not in a server dir, importing from server also doesn't work with the current linting rules. I would like to keep common for now and we can clean it up when migrating this file

src/plugins/data/server/plugin.ts Outdated
}

public setup(core: CoreSetup) {
public async setup(core: CoreSetup, { usageCollection }: DataPluginSetupDependencies) {
Copy link
Member

Choose a reason for hiding this comment

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

Is async necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not, I used to block on telemetry service setup during development, but it doesn't make sense in practice. I made the async-ness an implementation detail.

@flash1293 flash1293 requested a review from a team as a code owner December 20, 2019 15:07
flash1293
flash1293 commented Dec 20, 2019
View reviewed changes
docs/development/core/server/kibana-plugin-server.basepath.get.md
@@ -9,5 +9,5 @@ returns `basePath` value, specific for an incoming request.
<b>Signature:</b>

```typescript
get: (request: LegacyRequest | KibanaRequest<unknown, unknown, unknown, any>) => string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshdover These are the strange core API changes this PR introduces.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@flash1293 flash1293 merged commit 97ed566 into elastic:master Dec 20, 2019
@flash1293 flash1293 deleted the data-apis branch December 20, 2019 18:05
flash1293 added a commit to flash1293/kibana that referenced this pull request Dec 20, 2019
@flash1293
Migrate kql telemetry and scripts api (elastic#52651)
1cbf4e7
@flash1293 flash1293 mentioned this pull request Dec 20, 2019
Merged
flash1293 added a commit that referenced this pull request Dec 20, 2019
@flash1293
Migrate kql telemetry and scripts api (#52651) (#53717)
66222f0
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 8, 2020
@flash1293 @jkelastic
Migrate kql telemetry and scripts api (elastic#52651)
7076524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshdover joshdover joshdover approved these changes

@lukeelmers lukeelmers lukeelmers approved these changes

Assignees
No one assigned
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flash1293 @elasticmachine @lukeelmers @joshdover