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.

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

Merged
merged 6 commits into from
Sep 27, 2019

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Sep 4, 2019

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
  • requests.total: Number of requests processed
  • requests.bytes.received: Amount of data received, in bytes
  • requests.bytes.sent: Amount of data sent, in bytes
  • requests.processing.ms: Total time to process the requests
  • requests.errors.total: Number of errors
  • Threads
  • threads.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
  • cache.hit.total: The number of requests for resources that were served from the cache
  • cache.size.total: The current estimate of the cache size in kB
  • cache.size.max: The maximum permitted size of the cache in kB
  • cache.lookup.total: The number of requests for resources
  • cache.ttl.ms: The time-to-live for cache entries in milliseconds
  • Memory
  • memory.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:

@sayden sayden added enhancement Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Sep 4, 2019
@sayden sayden self-assigned this Sep 4, 2019
@andresrc andresrc added [zube]: Inbox in progress Pull request is currently in progress. [zube]: In Progress and removed [zube]: Inbox labels Sep 9, 2019
@sayden sayden added [zube]: In Review review and removed [zube]: In Progress in progress Pull request is currently in progress. labels Sep 13, 2019
@kaiyan-sheng
Copy link
Contributor

👍👍👍👍 How about https://github.com/elastic/beats/blob/master/x-pack/metricbeat/magefile.go#L93?

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.

Metrics collected LGTM, could you create a couple of data.json examples to see what events this module would generate?

x-pack/metricbeat/module/tomcat/README.md Outdated Show resolved Hide resolved
@sayden sayden added in progress Pull request is currently in progress. [zube]: In Progress and removed review [zube]: In Review labels Sep 18, 2019
@sayden sayden marked this pull request as ready for review September 20, 2019 11:01
@sayden sayden requested a review from a team as a code owner September 20, 2019 11:01
@sayden sayden added [zube]: In Review review and removed [zube]: In Progress in progress Pull request is currently in progress. labels Sep 20, 2019
*`tomcat.cache.size.total.kb`*::
+
--
The current estimate of the cache size in kB
Copy link
Contributor

@Titch990 Titch990 Sep 20, 2019

Choose a reason for hiding this comment

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

Suggested change
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").

Copy link
Contributor Author

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!

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.

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",
Copy link
Member

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.

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.

Copy link
Member

@jsoriano jsoriano Aug 25, 2020

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

Copy link
Member

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

Copy link

@dagwieers dagwieers Aug 25, 2020

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

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
Copy link
Member

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.

Suggested change
FROM tomcat:jdk13-openjdk-oracle
ARG TOMCAT_VERSION=9.0.26
FROM tomcat:${TOMCAT_VERSION}-jdk13-openjdk-oracle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh 😍 very good point

@kaiyan-sheng
Copy link
Contributor

👍👍👍👍 How about https://github.com/elastic/beats/blob/master/x-pack/metricbeat/magefile.go#L93?

@jsoriano just want to clarify here, is the change here needed? 😬

@jsoriano
Copy link
Member

+1+1+1+1 How about https://github.com/elastic/beats/blob/master/x-pack/metricbeat/magefile.go#L93?

@jsoriano just want to clarify here, is the change here needed? grimacing

It was more needed when we had one or two metricsets, it is not so important now.

@sayden
Copy link
Contributor Author

sayden commented Sep 27, 2019

Error is not related. Merging

@sayden sayden merged commit d0c34f9 into elastic:master Sep 27, 2019
@urso urso added the v7.5.0 label Oct 22, 2019
@andresrc
Copy link
Contributor

cf. #13860

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants