-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Metricbeat] Apache Tomcat lightweight module #13491
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.
Metrics collected LGTM, could you create a couple of data.json
examples to see what events this module would generate?
dc6e977
to
63b8fdb
Compare
metricbeat/docs/fields.asciidoc
Outdated
*`tomcat.cache.size.total.kb`*:: | ||
+ | ||
-- | ||
The current estimate of the cache size in kB |
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.
The current estimate of the cache size in kB | |
The current estimate of the cache size in kilobytes |
A tiny thing (and elsewhere too). KB is usually only used if it follows a number, otherwise it's usually spelled out in full (where "usually" means "as defined in the Microsoft Manual of Style").
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.
That's really interesting! I didn't know it. Fixed!
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.
Please use a fixed version for the docker image. For the rest it looks great to me.
}, | ||
"tomcat": { | ||
"cache": { | ||
"mbean": "Catalina:context=/docs,host=localhost,name=Cache,type=WebResourceRoot", |
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.
Umm, these mbeans should probably be placed in a common field, like jmx.mbean
. But this would be something to solve in the Jolokia module, or as part of #13604.
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.
@jsoriano We are looking into why the tomcat module is not working for us, and the issue is that the domain should be Tomcat
instead of Catalina
. I tried to override the mbean from its configuration file tomcat.yml, however it doesn't seem possible to set e.g. cache.mbean
or requests.mbean
ourselves.
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.
Hey @dagwieers, thanks for pointing this out. Do you know on what cases tomcat uses the Tomcat
domain and when the Catalina
one? I think the tomcat module should support both cases. Could you please open a new issue to add support for the Tomcat
domain?
In the meantime you can consider using the Jolokia module with a custom configuration: https://www.elastic.co/guide/en/beats/metricbeat/7.9/metricbeat-metricset-jolokia-jmx.html
If you are trying to override the mappings used by the tomcat module, I think but you should be using each metricset separatedly, and use the jmx.mappings
setting in the configuration for the overrides. But I am not sure if this would work well, I haven't tried it. It'd be for example something like this for the cache metricset:
- module: tomcat
metricsets: [cache]
jmx.mappings:
- mbean: 'Tomcat....'
attributes:
- attr: hitCount
field: hit.total
- attr: size
field: size.total.kb
- attr: maxSize
field: size.max.kb
- attr: lookupCount
field: lookup.total
- attr: ttl
field: ttl.ms
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.
Btw, it seems there was an attempt to change this also in the example configuration for Tomcat in the Prometheus jmx exporter: prometheus/jmx_exporter#92
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.
Do you know on what cases tomcat uses the Tomcat domain and when the Catalina one? I think the tomcat module should support both cases. Could you please open a new issue to add support for the Tomcat domain?
I am afraid I am not that experienced with Tomcat to tell you. But a way to influence that (or detect that) would be useful. I will open a ticket.
In the meantime you can consider using the Jolokia module with a custom configuration: https://www.elastic.co/guide/en/beats/metricbeat/7.9/metricbeat-metricset-jolokia-jmx.html
Yes, in the meantime I created my own config that uses jolokia and this basically removed the need to use the tomcat module altogether. Personally, I am unsure what the tomcat module brings to the table if it is just a standard listing for dashboards. We might as well go with just a tomcat.yml like your example. Here is the one that implements exactly the same:
# Ansible managed
# Module: jolokia
# Docs: https://www.elastic.co/guide/en/beats/metricbeat/7.9/metricbeat-module-jolokia.html
- module: jolokia
period: 10s
metricsets:
- jmx
hosts:
- localhost:8778
path: /jolokia/?ignoreErrors=true&canonicalNaming=false
namespace: tomcat
jmx.mappings:
# Reimplementation of tomcat/cache
- mbean: Tomcat:context=*,host=*,name=Cache,type=WebResourceRoot
attributes:
- attr: hitCount
field: cache.hit.total
- attr: size
field: cache.size.total.kb
- attr: maxSize
field: cache.size.max.kb
- attr: lookupCount
field: cache.lookup.total
- attr: ttl
field: cache.ttl.ms
# Reimplementation of tomcat/memory
- mbean: java.lang:type=Memory
attributes:
- attr: HeapMemoryUsage
field: memory.heap.usage
- attr: NonHeapMemoryUsage
field: memory.other.usage
# Reimplementation of tomcat/requests
- mbean: Tomcat:name=*,type=GlobalRequestProcessor
attributes:
- attr: requestCount
field: requests.total
- attr: bytesReceived
field: requests.bytes.received
- attr: bytesSent
field: requests.bytes.sent
- attr: processingTime
field: requests.processing.ms
- attr: errorCount
field: requests.errors.total
# Reimplementation of tomcat/threading
- mbean: Tomcat:name=*,type=ThreadPool
attributes:
- attr: currentThreadsBusy
field: threading.busy
- attr: maxThreads
field: threading.max
- attr: currentThreadCount
field: threading.current
- attr: keepAliveCount
field: threading.keep_alive.total
- attr: keepAliveTimeout
field: threading.keep_alive.timeout.ms
- mbean: java.lang:type=Threading
attributes:
- attr: TotalStartedThreadCount
field: threading.started.total
- attr: CurrentThreadUserTime
field: threading.user.time.ms
- attr: CurrentThreadCpuTime
field: threading.cpu.time.ms
- attr: ThreadCount
field: threading.total
- attr: PeakThreadCount
field: threading.peak
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.
BTW another concern I had was that I could not find the tomcat module in de codebase and it took some time to find this PR (given I did not have a reference). Now I see it is under x-pack, but that is non-trivial for people to find. And unfortunately searching for tomcat does not help ;-)
Maybe it would be worthwhile to merge modules together in the code, but split them for packaging/distribution/documentation instead?
@@ -0,0 +1,7 @@ | |||
FROM tomcat:jdk13-openjdk-oracle |
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.
Sorry I didn't see it before, we should fix a version here, and we might also add a build arg.
FROM tomcat:jdk13-openjdk-oracle | |
ARG TOMCAT_VERSION=9.0.26 | |
FROM tomcat:${TOMCAT_VERSION}-jdk13-openjdk-oracle |
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.
Ohhh 😍 very good point
@jsoriano just want to clarify here, is the change here needed? 😬 |
It was more needed when we had one or two metricsets, it is not so important now. |
6c6ca08
to
40f0215
Compare
40f0215
to
f5e4f0c
Compare
Error is not related. Merging |
cf. #13860 |
This is the metrics I think that we should add to the module. Some descriptions are missing in the docs but they are self-explanatory (We can add our own description but let's decide first what to add).
At the same time. I think we shouldn't do a single metricset:
requests.total
: Number of requests processedrequests.bytes.received
: Amount of data received, in bytesrequests.bytes.sent
: Amount of data sent, in bytesrequests.processing.ms
: Total time to process the requestsrequests.errors.total
: Number of errorsthreads.busy
:threads.max
:threads.current
:keep_alive.total
:keep_alive.timeout.ms
:thread.started.total
:thread.user.time.ms
:thread.cpu.time.ms
:thread.total
:thread.peak
:cache.hit.total
: The number of requests for resources that were served from the cachecache.size.total
: The current estimate of the cache size in kBcache.size.max
: The maximum permitted size of the cache in kBcache.lookup.total
: The number of requests for resourcescache.ttl.ms
: The time-to-live for cache entries in millisecondsmemory.heap.usage.committed
:memory.heap.usage.max
:memory.heap.usage.used
:memory.heap.usage.init
:memory.other.usage.committed
:memory.other.usage.max
:memory.other.usage.used
:memory.other.usage.init
: