-
Notifications
You must be signed in to change notification settings - Fork 177
Modernize the apache tomcat mixin #1493
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Structurally all good, though I left a couple comments about layout and potentially reusing common-lib components a bit more, rather than reusing the generic implementation of timeseries :)
+ g.panel.timeSeries.panelOptions.withDescription('The memory usage of the JVM of the instance.'), | ||
|
||
overviewCPUUsagePanel: | ||
commonlib.panels.generic.timeSeries.base.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an existing cpuUsage panel that you can use, instead of the generic timeseries panel. ideally we'd like to reuse as much of the common-lib as possible, e.g. https://github.com/grafana/jsonnet-libs/blob/master/common-lib/common/panels/cpu/timeSeries/utilization.libsonnet
There's a few options, worth having a look
|
||
// Overview Panels | ||
|
||
overviewMemoryUsagePanel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing as CPU below, we have pre-made stylised panels for memory: https://github.com/grafana/jsonnet-libs/tree/master/common-lib/common/panels/memory/timeSeries
+ g.panel.timeSeries.options.legend.withAsTable(true) | ||
+ g.panel.timeSeries.options.legend.withPlacement('right'), | ||
|
||
overviewRequestsPanel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually forgot about this in the past PRs, but we also have a couple premade options for request panels that would be good to use if any make sense (or if you make something custom, we can contribute it back to common-libs) https://github.com/grafana/jsonnet-libs/tree/master/common-lib/common/panels/requests/timeSeries (there's also stat versions)
overview: g.panel.row.new('Overview') | ||
+ g.panel.row.withCollapsed(value=false) | ||
+ g.panel.row.withPanels([ | ||
panels.overviewMemoryUsagePanel { gridPos+: { w: 12 } }, | ||
panels.overviewCPUUsagePanel { gridPos+: { w: 12 } }, | ||
panels.overviewTrafficSentPanel { gridPos+: { w: 12 } }, | ||
panels.overviewTrafficReceivedPanel { gridPos+: { w: 12 } }, | ||
panels.overviewRequestsPanel { gridPos+: { w: 12 } }, | ||
panels.overviewProcessingTimePanel { gridPos+: { w: 12 } }, | ||
panels.overviewThreadsPanel { gridPos+: { w: 24 } }, | ||
]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about converting the first four panels here to stat panels and reducing the footprint of this row a bit? Potentially adding a fifth stat panel for number of hosts? Number of clusters?
Basically I'm looking for something to break up the flow of the dashboard a bit, having the repeatable 2 column identical size panels leave the user with little reference when looking through, especially on smaller screens that have scrolling. Something to break the flow and make it less blocky would be super helpful.
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number of hosts? Number of clusters?
Yep I think those would be good value adds, will get that pushed up here momentarily!
What do you think about converting the first four panels here to stat panels and reducing the footprint of this row a bit
Hmm I do agree that the dashboard could definitely be repositioned to provide more separation and that suggestion does follow our best practices!
I might argue that seeing CPUUsage and MemoryUsage over a time window has a large value add especially when it comes to services like Tomcat where traffic may be spiky... Do you think it'd be redundant to show a stat/timeseries on the same dashboard? A stat to orient users if they're approaching thresholds, while also showing the signal over time to highlight any suspicious spikes.
I know there's the stat panel with the graphMode that shows the trend of data, but just have always preferred the timeseries when having multiple instances.
Maybe something like this?
| CPU stat | Memory Stat | Traffic Sent stat | Traffic Received stat | Number of hosts | Number of clusters |
| CPU timeseries | Memory timeseries |
| Processing Time | Threads |
Open to the idea though!
g.dashboard.variable.query.new('protocol', | ||
query='label_values(tomcat_bytesreceived_total{%(queriesSelector)s}, protocol)' % vars) + g.dashboard.variable.custom.selectionOptions.withMulti(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not sure if this is a linting/automatic formatting thing for a multiline bit of code, but the second line also exceeds the console width and ends up wrapped. Might be worth just folding it into one line?
Looks a bit strange in both git diff and IDEs
Updates the tomcat mixin to use the g.libsonnet and commonlib patterns.
Overview:
Hosts:

Logs:
