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

[Metricbeat] Add Overview dashboard to Tomcat module #14026

Merged
merged 1 commit into from
Feb 7, 2020
Merged

[Metricbeat] Add Overview dashboard to Tomcat module #14026

merged 1 commit into from
Feb 7, 2020

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Oct 11, 2019 •
edited
Loading

@sayden sayden requested a review from a team as a code owner October 11, 2019 14:53
@sayden sayden self-assigned this Oct 11, 2019
jsoriano
jsoriano reviewed Oct 14, 2019
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Selection of metrics and visualization LGTM. Some comments:

  • There are some continuously growing graphs, could we show the derivative instead?
  • In the screenshot the Total Requests title appears with the [Metricbeat Tomcat] suffix, though in the code it seems to be relabeled, could you confirm that in the final version the suffix doesn't appear?
  • Consider adding a screenshot, and if you do it capture it with the default theme 🙂

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
@sayden sayden force-pushed the feature/xp/mb/tomcat/overview-dashboard branch from 6614a4e to 2120f2e Compare October 15, 2019 14:27
@sayden
Copy link
Contributor Author

sayden commented Oct 16, 2019

jenkins, test this

jsoriano
jsoriano reviewed Oct 17, 2019
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we shouldn't mention the specifics of the calculations in the titles of the visualizations.

x-pack/metricbeat/module/tomcat/_meta/kibana/7/dashboard/Metricbeat-Tomcat-overview.json Outdated
},
"panelIndex": "6",
"panelRefName": "panel_4",
"title": "Derivative of total requests",
Copy link
Member

Choose a reason for hiding this comment

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

No need to mention in the visible titles that this is a derivative.

@sayden sayden force-pushed the feature/xp/mb/tomcat/overview-dashboard branch from 4a0a74a to 13bcac4 Compare October 18, 2019 10:47
@exekias exekias added needs_backport PR is waiting to be backported to other branches. v7.5.0 and removed needs_backport PR is waiting to be backported to other branches. v7.5.0 labels Oct 18, 2019
@andresrc
Copy link
Contributor

is there anything blocking this?

ping @sayden @jsoriano

@andresrc
Copy link
Contributor

Part of #13860

@jsoriano
Copy link
Member

I wouldn't say it is blocking but there is an unaddressed comment, I think we should title the visualizations with the value they are representing, not with the calculation made. e.g. something like Total requests or Requests over time, or Requests per second, instead of Derivative of total requests.

@andresrc andresrc added the Team:Integrations Label for the Integrations team label Dec 12, 2019
@sayden sayden force-pushed the feature/xp/mb/tomcat/overview-dashboard branch from 13bcac4 to c2c854a Compare January 6, 2020 14:14
@sayden
Copy link
Contributor Author

sayden commented Jan 7, 2020 •
edited
Loading

Ok, titles have been changed.

@jsoriano
Copy link
Member

jsoriano commented Jan 9, 2020

Ok, titles have been changed.

Thanks! Could you regenerate the screenshot?

@ChrsMark ChrsMark mentioned this pull request Jan 15, 2020
Closed
15 tasks
@andresrc andresrc added Team:Services (Deprecated) Label for the former Integrations-Services team and removed [zube]: In Review labels Jan 27, 2020
@sayden sayden force-pushed the feature/xp/mb/tomcat/overview-dashboard branch from e309d43 to c7dd262 Compare February 7, 2020 15:19
@sayden
Copy link
Contributor Author

sayden commented Feb 7, 2020

Screenshot updated

jsoriano
jsoriano approved these changes Feb 7, 2020
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks! It looks great.

@jsoriano jsoriano added needs_backport PR is waiting to be backported to other branches. v7.7.0 labels Feb 7, 2020
@sayden sayden merged commit 790036b into elastic:master Feb 7, 2020
@kaiyan-sheng kaiyan-sheng mentioned this pull request Jun 3, 2020
Merged
@kaiyan-sheng kaiyan-sheng added v7.9.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jun 3, 2020
kaiyan-sheng added a commit that referenced this pull request Jun 3, 2020
@kaiyan-sheng @sayden
Cherry-pick #14026 to 7.x: [Metricbeat] Add Overview dashboard to Tom…
3d9cefc 
…cat module (#18938)

* [Metricbeat] Add Overview dashboard to Tomcat module

(cherry picked from commit 790036b)

Co-authored-by: Mario Castro <mariocaster@gmail.com>
@sayden sayden deleted the feature/xp/mb/tomcat/overview-dashboard branch August 25, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsoriano jsoriano jsoriano approved these changes

Assignees

@sayden sayden

Labels
:Dashboards enhancement Metricbeat Metricbeat review Team:Integrations Label for the Integrations team Team:Services (Deprecated) Label for the former Integrations-Services team v7.7.0 v7.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sayden @andresrc @jsoriano @exekias @kaiyan-sheng