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

[types removal] Remove type from bulk_uploader in monitoring #32315

Merged
merged 3 commits into from
Mar 5, 2019
Merged

[types removal] Remove type from bulk_uploader in monitoring #32315

merged 3 commits into from
Mar 5, 2019

Conversation

sulemanof
Copy link
Contributor

Summary

This removes type from bulk_uploader requests in monitoring

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@epixa epixa requested a review from a team March 4, 2019 14:46
@epixa epixa added the Team:Monitoring Stack Monitoring team label Mar 4, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

chrisronline
chrisronline previously requested changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on!

Found a blocker though. See my comments

x-pack/plugins/monitoring/server/kibana_monitoring/bulk_uploader.js Outdated
@@ -181,7 +179,7 @@ export class BulkUploader {
const flat = Object.keys(typesNested).reduce((accum, type) => {
return [
...accum,
{ index: { _type: type } },
{ index: {} },
Copy link
Contributor

Choose a reason for hiding this comment

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

This will actually break internal collection. It's a bit confusing because the endpoint we use to index monitoring documents is a different endpoint than the typical bulk endpoint. This _type is actually rewritten in this custom ES endpoint to not be the type of the document, but rather a type field nested in the source of the indexed document. We need to remove this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrisronline in that case what is a way to avoid a deprecation warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisronline @sulemanof
I've created issue to track that: #32458.

x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_settings_collector.js
@@ -36,7 +36,6 @@ export async function getDefaultAdminEmail(config, callCluster, log) {
const version = config.get('pkg.version');
const uiSettingsDoc = await callCluster('get', {
index,
type: 'doc',
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks good!

@alexwizp alexwizp mentioned this pull request Mar 5, 2019
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof sulemanof requested a review from chrisronline March 5, 2019 09:50
@alexwizp alexwizp dismissed chrisronline’s stale review March 5, 2019 09:52

New issue was created: #32458. We can merge this pull request partially

@alexwizp alexwizp merged commit 748c18b into elastic:master Mar 5, 2019
@sulemanof sulemanof mentioned this pull request Mar 5, 2019
Merged
sulemanof added a commit to sulemanof/kibana that referenced this pull request Mar 5, 2019
@sulemanof
[types removal] Remove type from bulk_uploader in monitoring (elastic…
2484493 
…#32315)

* [types removal] Remove type from bulk_uploader in monitoring

* Remove type from get_settings_collector

* Revert changes
sulemanof added a commit to sulemanof/kibana that referenced this pull request Mar 5, 2019
@sulemanof
[types removal] Remove type from bulk_uploader in monitoring (elastic…
7966cd1 
…#32315)

* [types removal] Remove type from bulk_uploader in monitoring

* Remove type from get_settings_collector

* Revert changes
@sulemanof sulemanof mentioned this pull request Mar 5, 2019
Merged
sulemanof added a commit that referenced this pull request Mar 5, 2019
@sulemanof
[types removal] Remove type from bulk_uploader in monitoring (#32315) (…
2d2e061 
…#32465)

* [types removal] Remove type from bulk_uploader in monitoring

* Remove type from get_settings_collector

* Revert changes
sulemanof added a commit that referenced this pull request Mar 6, 2019
@sulemanof
[types removal] Remove type from bulk_uploader in monitoring (#32315) (…
b01791d 
…#32466)

* [types removal] Remove type from bulk_uploader in monitoring

* Remove type from get_settings_collector

* Revert changes
@rayafratkina rayafratkina added the release_note:skip Skip the PR/issue when compiling release notes label Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexwizp alexwizp alexwizp approved these changes

@tsullivan tsullivan Awaiting requested review from tsullivan

@epixa epixa Awaiting requested review from epixa

@jakelandis jakelandis Awaiting requested review from jakelandis

@joshdover joshdover Awaiting requested review from joshdover

@chrisronline chrisronline Awaiting requested review from chrisronline

Assignees
No one assigned
Labels
deprecation_warnings release_note:skip Skip the PR/issue when compiling release notes Team:Monitoring Stack Monitoring team v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sulemanof @elasticmachine @chrisronline @alexwizp @epixa @rayafratkina