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

[Monitoring] Remove kibana_stats.requests.status_codes from bulk uploader #20855

Merged
merged 3 commits into from
Jul 24, 2018
Merged

[Monitoring] Remove kibana_stats.requests.status_codes from bulk uploader #20855

merged 3 commits into from
Jul 24, 2018

Conversation

chrisronline
Copy link
Contributor

Resolves #20853

This PR removes an unused field from the bulk uploader which transports Kibana stats to ES. Tests have been updated and pass. I tested that monitoring docs indexed after this code change do not contain kibana_stats.requests.status_codes

@chrisronline chrisronline self-assigned this Jul 16, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ycombinator
Copy link
Contributor

I checked out this PR and did a global search for status_codes and a few occurrences still came up. Can you run the same search and audit the results?

@tsullivan
Copy link
Member

I checked out this PR and did a global search for status_codes and a few occurrences still came up. Can you run the same search and audit the results?

There may be reference to it in the mock data that goes through Metrics.capture. It is fine if it stays in mock data, as long as the capture method does not include it in its result

ycombinator
ycombinator approved these changes Jul 19, 2018
View reviewed changes
Copy link
Contributor

@ycombinator ycombinator 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.

Checked the stats API response before and after this PR (no changes, as expected). Also checked the docs indexed into .monitoring-kibana-6-* by bulk uploader before and after this PR. After this PR the status_code field is not indexed (as expected).

Code changes look good too. I noticed that you left references to status_codes in various mappings.json files in functional tests' es_archives. I think this is okay because the mappings in ES do indeed contain this field (incidentally mapped incorrectly).

Overall LGTM.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tsullivan
Copy link
Member

jenkins test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

tsullivan
tsullivan approved these changes Jul 23, 2018
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

@chrisronline chrisronline merged commit ade24f2 into elastic:master Jul 24, 2018
@chrisronline chrisronline deleted the monitoring/remove_status_codes branch July 24, 2018 14:15
@chrisronline chrisronline mentioned this pull request Jul 24, 2018
Merged
chrisronline added a commit to chrisronline/kibana that referenced this pull request Jul 24, 2018
@chrisronline
[Monitoring] Remove kibana_stats.requests.status_codes from bulk up…
4915405 
…loader (elastic#20855)

* Remove `status_codes` from bulk uploader

* Remove more references to `status_codes`
chrisronline added a commit that referenced this pull request Jul 24, 2018
@chrisronline
[Monitoring] Remove kibana_stats.requests.status_codes from bulk up…
d31e997 
…loader (#20855) (#21149)

* Remove `status_codes` from bulk uploader

* Remove more references to `status_codes`
@chrisronline
Copy link
Contributor Author

Backport:

6.x: d31e997

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsullivan tsullivan tsullivan approved these changes

@ycombinator ycombinator ycombinator approved these changes

Assignees

@chrisronline chrisronline

Labels
review Team:Monitoring Stack Monitoring team v6.4.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chrisronline @elasticmachine @ycombinator @tsullivan