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

Implement cluster metrics endpoint to fetch from all nodes #1990

Merged
merged 3 commits into from Mar 31, 2016

Conversation

Projects
None yet
2 participants
@bernd
Member

bernd commented Mar 29, 2016

Adjust frontend MetricsStore to use the new endpoint. Saves nodes - 1 requests in a multi node setup.

Fixes #1974

bernd added some commits Mar 29, 2016

Implement cluster metrics endpoint to fetch from all nodes
Adjust frontend MetricsStore to use the new endpoint. Saves nodes - 1 requests
in a multi node setup.

Fixes #1974

@bernd bernd added this to the 2.0.0 milestone Mar 29, 2016

@edmundoa edmundoa self-assigned this Mar 30, 2016

import static javax.ws.rs.core.Response.Status.BAD_GATEWAY;
@RequiresAuthentication
@Api(value = "Cluster/Metrics", description = "Cluster-wide Internal Graylog metrics")
@Path("/cluster/{nodeId}/metrics")
@Path("/cluster")

This comment has been minimized.

@edmundoa

edmundoa Mar 31, 2016

Member

This change merges this resource with ClusterSystemResource in swagger. Not sure if we want to fix it, but it feels a bit confusing at least.

This comment has been minimized.

@bernd

bernd Mar 31, 2016

Member

Hmm, good catch. I guess the only way out here is to create another resource class.

This comment has been minimized.

@bernd

bernd Mar 31, 2016

Member

Or use something like /cluster/_all/metrics/multiple to avoid the /cluster/metrics/multiple endpoint.

This comment has been minimized.

@edmundoa

edmundoa Mar 31, 2016

Member

I guess the renaming solution would be more consistent with the rest of the API, I don't think we use _all to indicate all nodes anywhere else.

});
const promise = this._allResults(promises).then((responses) => {
const url = URLUtils.qualifyUrl(ApiRoutes.ClusterMetricsApiController.multipleAllNodes().url);

This comment has been minimized.

@edmundoa

edmundoa Mar 31, 2016

Member

Do we really want to get all metrics for all nodes, even when we only need metrics from one of them? Going to System -> Nodes -> Node fetches metrics about that node, and all other nodes in the cluster, even though we are not using them.

This comment has been minimized.

@bernd

bernd Mar 31, 2016

Member

But we still request the throughput counter for every node. So I think it's a trade off between fetching a bit more data from all nodes than needed and doing separate requests for each node.

For the throughput counter we contact every node anyway, so we won't even save any requests to any nodes. The only drawback I see is that we request a bit more data than needed.

This comment has been minimized.

@edmundoa

edmundoa Mar 31, 2016

Member

I didn't think of that, sounds reasonable IMO 👍

@bernd

This comment has been minimized.

Member

bernd commented Mar 31, 2016

@edmundoa Please check the latest commits. I removed the class level @Path to make the UI browser work. (needed a tweak for the Generator code...)

@bernd

This comment has been minimized.

Member

bernd commented Mar 31, 2016

Damn, there is a problem. Sorry, hang on.

@bernd bernd force-pushed the fix-issue-1974 branch from 51dda88 to 1d044ab Mar 31, 2016

@bernd

This comment has been minimized.

Member

bernd commented Mar 31, 2016

@edmundoa I renamed the old cluster metrics endpoint to ClusterNodeMetricsResource and created a new ClusterMetricsResource class.

@edmundoa edmundoa merged commit 70a4228 into master Mar 31, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 779 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 268 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@edmundoa

This comment has been minimized.

Member

edmundoa commented Mar 31, 2016

LGTM 👍

@edmundoa edmundoa deleted the fix-issue-1974 branch Mar 31, 2016

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