Skip to content

[O11y][Apache Tomcat] Add integration package with thread pool data stream #6609

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
Merged
merged 13 commits into from
Jul 24, 2023
Merged

[O11y][Apache Tomcat] Add integration package with thread pool data stream #6609

merged 13 commits into from
Jul 24, 2023

Conversation

harnish-crest-data
Copy link
Contributor

@harnish-crest-data harnish-crest-data commented Jun 19, 2023 •
edited
Loading

Urgency

  • Normal

Activity Type

  • Enhancement

What does this PR do?

  • Added 1 data stream (Thread Pool metrics).
  • Added data collection logic for the data streams.
  • Added the ingest pipeline for the data streams.
  • Mapped fields according to the ECS schema and added Fields metadata in the appropriate yaml files.
  • Added dashboards and visualizations.
  • Added system test cases for the data stream.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.
  • Verify if field mapping is correct in the data stream template.
  • Verification of data in visualisation after enabling TSDB flag in kibana
  • Verification of the count of documents (before & after TSDB enablement) in Discover Interface

How to test this PR locally

  • Clone integrations repo.
  • Install elastic-package locally.
  • Start elastic stack using elastic-package.
  • Move to integrations/packages/apache_tomcat) directory.
  • Run the following command to run tests. elastic-package test

Meta issue

Related Issues

Related PRs

@harnish-crest-data harnish-crest-data self-assigned this Jun 19, 2023
@elasticmachine
Copy link

elasticmachine commented Jun 19, 2023 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-07-20T05:59:55.606+0000

  • Duration: 23 min 7 sec

Test stats 🧪

Test Results
Failed 0
Passed 47
Skipped 0
Total 47

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Jun 19, 2023 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (8/8) 💚
Files 100.0% (8/8) 💚 2.767
Classes 100.0% (8/8) 💚 2.767
Methods 94.595% (70/74) 👍 2.422
Lines 82.774% (764/923) 👎 -8.376
Conditionals 100.0% (0/0) 💚

@harnish-crest-data harnish-crest-data marked this pull request as ready for review June 21, 2023 05:34
@harnish-crest-data harnish-crest-data requested a review from a team as a code owner June 21, 2023 05:34
This was referenced Jun 29, 2023
Closed
Merged
@harnish-crest-data harnish-crest-data mentioned this pull request Jul 3, 2023
Closed
13 tasks
@kush-elastic kush-elastic mentioned this pull request Jul 3, 2023
Merged
7 tasks
This was referenced Jul 3, 2023
Merged
Closed
Closed
@ishleenk17
Copy link
Member

@agithomas : Please check from TSDB perspective.
@SubhrataK : Please check the dashboards.

@harnish-crest-data harnish-crest-data mentioned this pull request Jul 17, 2023
Merged
4 tasks
@ishleenk17
Copy link
Member

Looks good. Post TSDB we can merge the changes

@agithomas
Copy link
Contributor

@agithomas : Please check from TSDB perspective. @SubhrataK : Please check the dashboards.

@SubhrataK , @ishleenk17 , @milan-elastic : Please have a look at the dashboard screenshot. The Active thread vs current thread looks confusing to me. Kindly review this part.

agithomas
agithomas reviewed Jul 19, 2023
View reviewed changes
agithomas
agithomas approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM from TSDB side of things. Hence approving.
Please look into other comments as well.

@harnish-crest-data
Copy link
Contributor Author

@agithomas : Please check from TSDB perspective. @SubhrataK : Please check the dashboards.

As per @SubhrataK suggestions, I am going to update the dashboard. Please refer the following dashboard.
thread_pool

@SubhrataK , @ishleenk17 , @milan-elastic : Please have a look at the dashboard screenshot. The Active thread vs current thread looks confusing to me. Kindly review this part.

@agithomas, The Active threads represent Current active threads at JVM level (from java.lang:type=Threading) and the Current threads represent Current number of threads, taken from the ThreadPool that is mentioned in the fields.yml file. I discussed with @milan-elastic the same, We'll add this information in the line chart as well for better understanding.

@harnish-crest-data
address review comments related to dashboard and readme
5e11275
@harnish-crest-data
Merge branch 'main' of https://github.com/harnish-elastic/integrations …
9e7d06a 
…into package-apache_tomcat_thread_pool

Conflicts:
	packages/apache_tomcat/_dev/build/docs/README.md
	packages/apache_tomcat/changelog.yml
	packages/apache_tomcat/docs/README.md
	packages/apache_tomcat/manifest.yml
@harnish-crest-data harnish-crest-data force-pushed the package-apache_tomcat_thread_pool branch from e6cb9dd to 9e7d06a Compare July 20, 2023 05:59
@ishleenk17
Copy link
Member

The Active threads represent Current active threads at JVM level (from java.lang:type=Threading) and the Current threads represent Current number of threads, taken from the ThreadPool that is mentioned in the fields.yml file. I discussed with @milan-elastic the same, We'll add this information in the line chart as well for better understanding.

@harnish-elastic : As discussed if this clarity is given to the user at the dashboard we should be good to keep both the values active and current.
Please share the dashboard screenshot here.

ishleenk17
ishleenk17 approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

Looks good!

@harnish-crest-data
Copy link
Contributor Author

@harnish-elastic : As discussed if this clarity is given to the user at the dashboard we should be good to keep both the values active and current. Please share the dashboard screenshot here.

Please find the updated description for the Thread distribution by server over time graph
image

@harnish-crest-data harnish-crest-data merged commit 6c42b51 into elastic:main Jul 24, 2023
@elasticmachine
Copy link

Package apache_tomcat - 0.10.0 containing this change is available at https://epr.elastic.co/search?package=apache_tomcat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agithomas agithomas agithomas approved these changes

@ishleenk17 ishleenk17 ishleenk17 approved these changes

@milan-elastic milan-elastic milan-elastic approved these changes

@kush-elastic kush-elastic Awaiting requested review from kush-elastic

Assignees

@harnish-crest-data harnish-crest-data

Labels
enhancement New feature or request Integration:apache_tomcat Apache Tomcat v8.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@harnish-crest-data @elasticmachine @ishleenk17 @agithomas @milan-elastic