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

Add missing TaskExecutorResource to jmx #22914

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

JRemitz
Copy link
Member

@JRemitz JRemitz commented Aug 1, 2024

Description

This is an attempt to fix #22905 which after #18118 the TaskExecutor class is moved to an abstract implementation with the detail used is now in the TimeSharingTaskExecutor class. The intention of this PR is to add this class into the exposed jmx mbean.

Also, the TaskExecutor is exposed in the mbean with empty metrics so I've removed that as well. I'm unclear if ThreadPerDriverTaskExecutor should have any jmx metrics as an alternative TaskExecutor, but since JMX wasn't referenced, I didn't include that in the if block.

Additional context and related issues

I did notice that beans are now also prefixed with jmx.trino... in the domains. Is that intended? Previous model was just trino.... I may be mistaken but this is how they're surfaced in Datadog if I include the additional timesharing metrics from MultilevelSplitQueue which are exposed in mbean.
EDIT: I've confused myself. That alias is done via the Datadog Trino integration. I'll also look into updating that as well.

Nothing additional to add here except that I don't know if this does everything I'm intending to. Just trying to push it along!

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
() Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:

Add

# Section
* Add JMX metrics for the bean, `trino.execution.executor.timesharing:name=TimeSharingTaskExecutor`.  This replaces metrics previously found in `trino.execution.executor:name=TaskExecutor`

Copy link

cla-bot bot commented Aug 1, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented Aug 1, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@hashhar hashhar requested review from martint and wendigo August 2, 2024 08:13
@hashhar
Copy link
Member

hashhar commented Aug 14, 2024

@martint PTAL.

@JRemitz JRemitz requested a review from hashhar August 23, 2024 16:53
@martint
Copy link
Member

martint commented Aug 26, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Aug 26, 2024
Copy link

cla-bot bot commented Aug 26, 2024

The cla-bot has been summoned, and re-checked this pull request!

@martint martint merged commit 0b9a103 into trinodb:master Aug 26, 2024
96 checks passed
@github-actions github-actions bot added this to the 455 milestone Aug 26, 2024
@JRemitz JRemitz deleted the bug/jremitz-missing_jmx_metrics branch August 26, 2024 18:14
@wwbbcjeyc
Copy link

After upgrading to v460 JMX beam can't find 'trino. Execution. Executor. Timesharing: name = TimeSharingTaskExecutor' @JRemitz

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

Successfully merging this pull request may close these issues.

Trino JMX RMI registry is missing splits metrics
4 participants