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

Expose monitoring.cluster_uuid in State API #13254

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Aug 15, 2019

Related: #13182.

This PR makes two (related) changes:

  • Exposes the monitoring.cluster_uuid setting value (if set) in the Beats State HTTP API (GET /state).
  • Consumes this field from the API response in the beat Metricbeat module's state and stats metricsets.

Testing this PR

  1. Run Elasticsearch. To start with a clean slate, delete .monitoring-* indices.
  2. Build Filebeat with this PR. Set monitoring.cluster_uuid in filebeat.yml to foobar. Also set http.enabled to true. Run Filebeat.
  3. Call the Filebeat State API: http://localhost:5066/state. Verify that monitoring.cluster_uuid is set to foobar in the response.
  4. Build Metricbeat with this PR. Enable the beat-xpack module. Run Metricbeat.
  5. Check that .monitoring-beats-* indices are created in Elasticsearch.
  6. Check that documents in .monitoring-beats-* indices contain the cluster_uuid field with value as foobar.

@ycombinator ycombinator force-pushed the libbeat-api-expose-monitoring-cluster-uuid branch from dcbf9e6 to 02e5203 Compare September 3, 2019 21:56
@ycombinator ycombinator marked this pull request as ready for review September 3, 2019 22:05
@ycombinator ycombinator requested a review from a team as a code owner September 3, 2019 22:05
@ycombinator ycombinator removed the request for review from a team September 3, 2019 22:06
@ycombinator ycombinator added in progress Pull request is currently in progress. v7.3.2 v8.0.0 v7.4.0 and removed in progress Pull request is currently in progress. labels Sep 3, 2019
@ycombinator ycombinator changed the title Expose monitoring.cluster_uuid in State API [WIP] Expose monitoring.cluster_uuid in State API Sep 3, 2019
@ycombinator ycombinator changed the title [WIP] Expose monitoring.cluster_uuid in State API Expose monitoring.cluster_uuid in State API Sep 3, 2019
@ycombinator ycombinator added review and removed in progress Pull request is currently in progress. labels Sep 3, 2019
@ycombinator ycombinator requested review from cachedout and a team September 3, 2019 22:34
@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring

@ycombinator
Copy link
Contributor Author

jenkins, test this

Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

This looks good, though I am wondering if this would be a candidate for some tests just to ensure this behavior doesn't regress in the future?

@ycombinator
Copy link
Contributor Author

ycombinator commented Sep 6, 2019

@cachedout I agree that we need tests around this change. So I started looking into writing tests for the libbeat API server and am planning to put up a separate PR for it.

We currently have no tests for libbeat APIs. Looking at the API server code, it may require some refactoring to make it more testable. Specifically, it's easy to spin up the API server but most APIs return data from registries, and these don't get populated in a test context, so all API responses are returned as null. I'd like to look into a few different options here: a) can we refactor the API server code to inject registries so we can setup some mocks in tests? Given that registries are of global scope in libbeat, does this even make sense? b) should we try to spin up an entire mock beat so registries are automatically populated, and an API server is started up which we can then test against? c) should we write the tests in python instead of golang? Python might make it easier to spin up a mockbeat but I believe we're trying to write more such "helper" code in golang going forward.

I expect this separate PR to take a bit of time (given what I noted above) and also potentially be comparatively larger than this one here. Are you okay with merging this PR here as-is and tracking tests in a separate issue (and PR)?

@cachedout
Copy link
Contributor

Hi @ycombinator

Regarding the testing-specific questions, I think it would be good to bring @ph and @urso into the conversation.

I am fine with merging this as-is and tracking the testing concerns in another issue. I will mark this as approved so we can move forward with getting this in.

@ycombinator
Copy link
Contributor Author

Regarding the testing-specific questions, I think it would be good to bring @ph and @urso into the conversation.

Yup, once I put up a PR for the tests I'll be adding them as reviewers so they can help me course correct and choose the right approach.

@ycombinator ycombinator merged commit 44e77e3 into elastic:master Sep 10, 2019
@ycombinator ycombinator deleted the libbeat-api-expose-monitoring-cluster-uuid branch September 10, 2019 14:05
@ycombinator
Copy link
Contributor Author

Issue to track tests: #13578.

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