Enhance GET _node/stats/pipelines API for Metricbeat monitoring by cachedout · Pull Request #10576 · elastic/logstash

cachedout · 2019-03-21T23:06:21Z

This will fail tests until #10561 is merged in and this PR is rebased. I'm just parking it here until then. ;)

ycombinator · 2019-05-06T12:56:57Z

@cachedout Looks like this PR now needs a merge/rebase from master.

author Guy Boertje <guy@elastic.co> 1556806171 +0100 committer Mike Place <mike.place@elastic.co> 1557234770 +0200 Bump JrJackson to 0.4.8 Fixes elastic#10748 LIR serializer refactor Remove commented code Remove more commented code Remove license and add encoding Style change to make code more vertical. eid and hash Use pipelines_info to construct the stats Add tests for new fields Add queue stats

cachedout · 2019-05-10T15:30:35Z

@jsvd and @yaauie I believe the only remaining test failure is unrelated to this PR so I consider it ready for review. @ycombinator is currently doing some functional testing on it as well.

ycombinator · 2019-05-10T15:31:49Z

Hi @cachedout, I noticed a few things after running this PR:

Per Enhance GET _node/stats/pipelines API for Metricbeat monitoring #10120 (comment), the vertices field should only be returned if ?vertices=true is set. At the moment the vertices field is always returned in the API response.
If you look at the sample JSON document in Enhance GET _node/stats/pipelines API for Metricbeat monitoring #10120 (comment), particularly the queue field, you'll notice it looks like this, containing 4 fields under it:
```
queue":{
  "type":"memory",
  "queue_size_in_bytes":0,
  "max_queue_size_in_bytes":0,
  "events_count":0
}
```
However, the queue field in the API response only contains the type field under it. Would it be possible to include the other 3 fields as well?
This is a new requirement so feel free to defer it to a follow up PR. If the elasticsearch output plugin is being used in a pipeline, could we add cluster_uuids to that pipeline's object in the API response, similar to what we did in GET /_node/pipelines API response?

ycombinator · 2019-05-20T14:03:01Z

@cachedout I pulled down this PR and re-tested it. Looks like points 1 and 2 from my previous comment have been addressed but point 3 hasn't yet. [EDIT] Please let me know if you plan to address it in this PR or a follow up PR.

cachedout · 2019-05-24T09:37:39Z

@ycombinator Sorry for the confusion. I was going to address the third point in a follow-up PR.

ycombinator

Thanks @cachedout. In that case, this PR LGTM functionally.

cachedout · 2019-05-27T08:42:26Z

This is ready for a review from @jsvd or @yaauie or somebody else on @elastic/logstash

danhermann · 2019-05-28T18:34:19Z

We are going to need to commit the changes from #10561 into master since they were merged only into the 7.x branch. Is there a plan for addressing the tech debt introduced in #10561?

cachedout · 2019-05-29T08:35:53Z

@danhermann By "tech debt" are you referring to this comment?

danhermann · 2019-05-29T18:08:57Z

@danhermann By "tech debt" are you referring to this comment?

Both that one and the earlier comment about tighter coupling than we would like between Logstash core and plugins.

cachedout · 2019-06-03T11:06:33Z

Hi @danhermann !

Forgive me, but I'm a bit confused by this:

We are going to need to commit the changes from #10561 into master since they were merged only into the 7.x branch.

The changes made by the commits in that PR do seem to be present in the master branch. For example, see the git blame here which seems to show these commits present in the master branch. Just in case, I tried creating a PR for but while the commits themselves apply cleanly, such a PR seems to result in no proposed changes to the files in the Logstash master branch. I'm not sure what I'm missing here so any help would be appreciated. :)

Regarding the comments about tight coupling, my understanding is that those were resolved via the introduction of the PluginMetadata system which was merged here and subsequent use of the new system in my changes. Did I misunderstand the nature of the tight-coupling problem? If you could provide more specifics about the nature of this problem which remain unresolved, I'd be happy of course to try and resolve them. :)

danhermann · 2019-06-03T16:14:29Z

The changes made by the commits in that PR do seem to be present in the master branch.

My apologies, @cachedout. I saw those changes first as a series of distinct commits in the 7.x branch and missed that they had been committed into master as a single, squashed commit (as we prefer).

Regarding the comments about tight coupling, my understanding is that those were resolved via the introduction of the PluginMetadata system which was merged here and subsequent use of the new system in my changes. Did I misunderstand the nature of the tight-coupling problem? If you could provide more specifics about the nature of this problem which remain unresolved, I'd be happy of course to try and resolve them. :)

Let me take another look at those and get back to you in a day or two if that's ok.

yaauie

I think we're in a pretty good spot here.

I have left a comment about retaining the @api private comment on our helper method and repeated a suggestion to move the method declaration to below the public API methods.

Beyond that, everything is looking good.

logstash-core/lib/logstash/api/commands/stats.rb

yaauie

👍

LGTM. Thanks for all the work to wrangle this through, @cachedout

cachedout · 2019-06-26T14:12:08Z

Thanks again for your help @yaauie. It's very much appreciated by me and the whole team. :)

@ycombinator Just as an FYI, the backport to this is here: #10909

…tic#10576) * parent 8c5697c author Guy Boertje <guy@elastic.co> 1556806171 +0100 committer Mike Place <mike.place@elastic.co> 1557234770 +0200 Bump JrJackson to 0.4.8 Fixes elastic#10748 LIR serializer refactor Remove commented code Remove more commented code Remove license and add encoding Style change to make code more vertical. eid and hash Use pipelines_info to construct the stats Add tests for new fields Add queue stats * bad merge resolution * bad merge resolution * Don't merge if nil * Better merge strategy * add vertex gate * Guard against nil * Use extended queue stats in pipeline report * Add cluster uuids to Elasticsearch outputters in pipeline output * move uuid * remove old uuid lookup * Only populate cluster_uuids when present * remove print * cluster_uuids -> cluster_uuid * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Make var singular * Match singular var name * Remove unnecessary nil check * Pass in the matching pipeline for the report * Remove old way of inserting cluster_uuids * Update logstash-core/lib/logstash/api/commands/stats.rb I like this much better and in testing it seems to work correctly. Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Remove unreferenced code that was part of debugging * Remove events var which was unused * Don't try to remove before insert * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Make pipeline extended stats generation more efficient * Implement suggestion to improve readability * Cleaner merging per review recommendation * Only generate extended_stats once * remove unneeded comments * Add cluster_uuid to node vertex * remove top-level cluster_uuids * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Implement change to make logic more simple suggested in review * Rely on options gate to insert graph Resolves concern here: elastic#10576 (comment) * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Move UUID lookup to API layer * Move private method to bottom per review recommandation

…) (#10909) * parent 8c5697c author Guy Boertje <guy@elastic.co> 1556806171 +0100 committer Mike Place <mike.place@elastic.co> 1557234770 +0200 Bump JrJackson to 0.4.8 Fixes #10748 LIR serializer refactor Remove commented code Remove more commented code Remove license and add encoding Style change to make code more vertical. eid and hash Use pipelines_info to construct the stats Add tests for new fields Add queue stats * bad merge resolution * bad merge resolution * Don't merge if nil * Better merge strategy * add vertex gate * Guard against nil * Use extended queue stats in pipeline report * Add cluster uuids to Elasticsearch outputters in pipeline output * move uuid * remove old uuid lookup * Only populate cluster_uuids when present * remove print * cluster_uuids -> cluster_uuid * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Make var singular * Match singular var name * Remove unnecessary nil check * Pass in the matching pipeline for the report * Remove old way of inserting cluster_uuids * Update logstash-core/lib/logstash/api/commands/stats.rb I like this much better and in testing it seems to work correctly. Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Remove unreferenced code that was part of debugging * Remove events var which was unused * Don't try to remove before insert * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Make pipeline extended stats generation more efficient * Implement suggestion to improve readability * Cleaner merging per review recommendation * Only generate extended_stats once * remove unneeded comments * Add cluster_uuid to node vertex * remove top-level cluster_uuids * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Implement change to make logic more simple suggested in review * Rely on options gate to insert graph Resolves concern here: #10576 (comment) * Update logstash-core/lib/logstash/api/commands/stats.rb Co-Authored-By: Ry Biesemeyer <yaauie@users.noreply.github.com> * Move UUID lookup to API layer * Move private method to bottom per review recommandation

…s for _node/stats Avoid to reassing the subdocument for queue metrics preferring a merge With PR elastic#10576 the PluginsStats.report(stats) overwrites the subsection related to queue instead of merge with newly created entries.

…s for _node/stats (#11923) Avoid to reassing the subdocument for queue metrics preferring a merge With PR #10576 the PluginsStats.report(stats) overwrites the subsection related to queue instead of merge with newly created entries.

…s for _node/stats Avoid to reassing the subdocument for queue metrics preferring a merge With PR elastic#10576 the PluginsStats.report(stats) overwrites the subsection related to queue instead of merge with newly created entries.

…s for _node/stats Avoid to reassing the subdocument for queue metrics preferring a merge With PR #10576 the PluginsStats.report(stats) overwrites the subsection related to queue instead of merge with newly created entries. Fixes #11970

…s for _node/stats Avoid to reassing the subdocument for queue metrics preferring a merge With PR elastic#10576 the PluginsStats.report(stats) overwrites the subsection related to queue instead of merge with newly created entries.

…s for _node/stats Avoid to reassing the subdocument for queue metrics preferring a merge With PR #10576 the PluginsStats.report(stats) overwrites the subsection related to queue instead of merge with newly created entries.

cachedout requested review from yaauie and ycombinator March 21, 2019 23:06

ycombinator mentioned this pull request Mar 28, 2019

Extend logstash.node_stats metricset for logstash_stats stack monitoring data elastic/beats#11511

Merged

This was referenced Apr 3, 2019

Make GET _node/pipelines/:id and GET _node/stats/pipelines/:id APIs return Elasticsearch output cluster_uuid #10602

Closed

Enhance GET _node/stats API for Metricbeat monitoring #10610

Closed

cachedout self-assigned this Apr 8, 2019

cachedout added the v7.1.0 label Apr 24, 2019

jsvd added v7.2.0 and removed v7.1.0 labels May 1, 2019

cachedout force-pushed the issue_101120 branch from 01f9f93 to 056296c Compare May 7, 2019 13:16

cachedout added 4 commits May 10, 2019 11:49

bad merge resolution

fe2d7b8

bad merge resolution

e9e4b69

Don't merge if nil

8bbb8c9

Better merge strategy

46b0c87

cachedout added 3 commits May 10, 2019 18:29

add vertex gate

ffe9108

Guard against nil

b2eb1b9

Use extended queue stats in pipeline report

6c1025b

ycombinator approved these changes May 24, 2019

View reviewed changes

yaauie reviewed Jun 21, 2019

View reviewed changes

logstash-core/lib/logstash/api/commands/stats.rb Outdated Show resolved Hide resolved

Move private method to bottom per review recommandation

498fbc9

yaauie approved these changes Jun 21, 2019

View reviewed changes

cachedout merged commit 252d5e7 into elastic:master Jun 22, 2019

ycombinator added v8.0.0 backport needed labels Jun 22, 2019

This was referenced Jun 22, 2019

Enhance GET _node/stats/pipelines API for Metricbeat monitoring #10120

Closed

GET _node/pipelines?graph=true API response no longer returns cluster_uuid #10883

Closed

This was referenced Jun 24, 2019

Inject hash and ephemeral_id into stats #10885

Merged

Restore UUID lookup to node #10884

Merged

cachedout added a commit to cachedout/logstash that referenced this pull request Jun 26, 2019

Backport of PR elastic#10576

a710bc7

cachedout mentioned this pull request Jun 26, 2019

Backport #10576 to 7.x #10908

Closed

cachedout mentioned this pull request Jun 26, 2019

[BACKPORT] Enhance GET _node/stats/pipelines API for Metricbeat monitoring (#10576) #10909

Merged

cachedout removed the backport needed label Jun 27, 2019

cachedout mentioned this pull request Jul 17, 2019

Enhanced API testing #10972

Merged

5 tasks

redbaron4 mentioned this pull request Aug 20, 2019

Missing stats in /_node/stats/pipelines after upgrade to 7.3.0 #11021

Closed

andsel mentioned this pull request May 25, 2020

Re-enable PQ integration tests to exercise the persistent feature #11921

Closed

2 tasks

andsel mentioned this pull request May 25, 2020

Exposed again the pipelines queue.data and queue.capacity subdocuments for _node/stats #11923

Merged

andsel mentioned this pull request Jun 3, 2020

Pass FEATURE_FLAG as Docker environment variable #11922

Merged

jsvd removed the v8.0.0 label Oct 31, 2021

Conversation

cachedout commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycombinator commented May 6, 2019

Uh oh!

cachedout commented May 10, 2019

Uh oh!

ycombinator commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycombinator commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cachedout commented May 24, 2019

Uh oh!

ycombinator left a comment

Choose a reason for hiding this comment

Uh oh!

cachedout commented May 27, 2019

Uh oh!

danhermann commented May 28, 2019

Uh oh!

cachedout commented May 29, 2019

Uh oh!

danhermann commented May 29, 2019

Uh oh!

cachedout commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danhermann commented Jun 3, 2019

Uh oh!

yaauie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yaauie left a comment

Choose a reason for hiding this comment

Uh oh!

cachedout commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cachedout commented Mar 21, 2019 •

edited

Loading

ycombinator commented May 10, 2019 •

edited

Loading

ycombinator commented May 20, 2019 •

edited

Loading

cachedout commented Jun 3, 2019 •

edited

Loading

cachedout commented Jun 26, 2019 •

edited

Loading