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

Move Logstash node_stats under node.stats #6714

Merged
merged 2 commits into from
Apr 4, 2018

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Mar 30, 2018

  • This is the same as for elasticsearch
  • Update data.json accordingly
  • Update Logstash version to 6.2.3
  • Update data.json generator to support namespace setting

@@ -78,7 +78,6 @@ func StandardizeEvent(ms mb.MetricSet, e mb.Event, modifiers ...mb.EventModifier
}

e.Timestamp = startTime
e.Namespace = ms.Registration().Namespace
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewkroh Can you check if this change has unintended side effects on the auditbeat side?

Copy link
Member

Choose a reason for hiding this comment

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

This will affect MetricSets that provide a Namespace at registration time. I think that by adding a conditional this should be safe.

if e.Namespace == "" {
    e.Namespace = ms.Registration().Namespace
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewkroh Add the if clause and switched node_stats to also use the registry for the namespace as it's constant. I will do the same for node_stats on the ES side.

@@ -73,6 +73,7 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
- Support apache status pages for versions older than 2.4.16. {pull}6450[6450]
- Add support for huge pages on Linux. {pull}6436[6436]
- Refactor docker CPU calculations to be more consistent with `docker stats`. {pull}6608[6608]
- Update logstash.node_stats metricset to write data under `logstash.docker.stats.*`. {pull}[]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant logstash.node.stats.* not logstash.docker.stats.*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ups, fixed.

@@ -1,4 +1,4 @@
FROM docker.elastic.co/logstash/logstash:6.0.0
FROM docker.elastic.co/logstash/logstash:6.2.3
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to figure out a way to proactively tests on the snapshots release for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is a general thing we have to come up with a solution, meaning testing each module against multiple versions.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Small changes required in the changelog

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Missing @andrewkroh answer on this.

@ruflin ruflin force-pushed the logstash-nodestats branch 3 times, most recently from 736cc8d to 543ae9f Compare April 3, 2018 08:23
@ruflin
Copy link
Contributor Author

ruflin commented Apr 3, 2018

Looks like the node_stats test fail. I probably have to adjust some setting, will investigate.

@andrewkroh
Copy link
Member

Looks like the failure is due to fields.yml.

Key 'logstash.node.stats.events.in' found in event is not documented!

ruflin added 2 commits April 4, 2018 09:04
* This is the same as for elasticsearch
* Update data.json accordingly
* Update Logstash version to 6.2.3
* Update data.json generator to support namespace setting
@ruflin ruflin force-pushed the logstash-nodestats branch from 543ae9f to 0bf0b4f Compare April 4, 2018 07:06
@ruflin
Copy link
Contributor Author

ruflin commented Apr 4, 2018

@andrewkroh @ph Tests should be fixed now. Ready for an other round.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

LGTM

@ph ph merged commit 604e0ba into elastic:master Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants