You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@livy.apache.org by "samuhasavak5 (via GitHub)" <gi...@apache.org> on 2026/08/14 14:43:01 UTC
[PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
samuhasavak5 opened a new pull request, #543:
URL: https://github.com/apache/livy/pull/543
## What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
(Include a link to the associated JIRA and make sure to add a link to this pr on the JIRA as well)
## How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
Please review https://livy.apache.org/community/ before opening a pull request.
## Was this patch authored or co-authored using generative AI tooling?
(If generative AI tooling has been used in the process of authoring this patch, please include the phrase:)
('Generated-by: ' followed by the name of the tool and its version.)
(If no, write 'No'.)
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
Posted by "roczei (via GitHub)" <gi...@apache.org>.
roczei commented on PR #543:
URL: https://github.com/apache/livy/pull/543#issuecomment-5396069130
@samuhasavak5
Thanks for the PR! Tested it locally and was able to query the endpoint successfully at http://localhost:8998/metrics/metrics?pretty=true.
Could you also update `docs/rest-api.md` to cover the new metrics? It looks like the `/metrics` endpoint and the new gauge names aren't documented yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on code in PR #543:
URL: https://github.com/apache/livy/pull/543#discussion_r3804319719
##########
server/src/main/scala/org/apache/livy/server/LivyServer.scala:
##########
@@ -252,6 +252,8 @@ class LivyServer extends Logging {
}
context.mountMetricsAdminServlet("/metrics")
+ new LivySessionMetrics(
+ interactiveSessionManager, batchSessionManager, metricRegistry)
Review Comment:
The choice of syntax seems a bit unusual here, because (if I understand correctly) what ends up happening is `metricRegistry.register()` gets called multiple times, and the resulting `LivySessionMetrics` reference is not stored anywhere. Is this the convention with Codehale gauge setup, or why is `LivySessionMetrics` not an object for example and why is `metricRegistry` the last argument, even though that is what is mostly being operated on? I think those changes could make the code more understandable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal merged PR #543:
URL: https://github.com/apache/livy/pull/543
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
Posted by "samuhasavak5 (via GitHub)" <gi...@apache.org>.
samuhasavak5 commented on code in PR #543:
URL: https://github.com/apache/livy/pull/543#discussion_r3815452902
##########
server/src/main/scala/org/apache/livy/server/LivyServer.scala:
##########
@@ -252,6 +252,8 @@ class LivyServer extends Logging {
}
context.mountMetricsAdminServlet("/metrics")
+ new LivySessionMetrics(
+ interactiveSessionManager, batchSessionManager, metricRegistry)
Review Comment:
I have addressed all comments in this commit 810bd2dc:
1. Refactored `LivySessionMetrics` to an `object` with `register(...)`.
2. Moved `metricRegistry` to the first parameter.
3. Updated the call site in `LivyServer.scala` accordingly.
All 8 tests in `LivySessionMetricsSpec` pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
Posted by "samuhasavak5 (via GitHub)" <gi...@apache.org>.
samuhasavak5 commented on code in PR #543:
URL: https://github.com/apache/livy/pull/543#discussion_r3815452902
##########
server/src/main/scala/org/apache/livy/server/LivyServer.scala:
##########
@@ -252,6 +252,8 @@ class LivyServer extends Logging {
}
context.mountMetricsAdminServlet("/metrics")
+ new LivySessionMetrics(
+ interactiveSessionManager, batchSessionManager, metricRegistry)
Review Comment:
I have addressed your comments in commit 810bd2dc:
1. Refactored `LivySessionMetrics` to an `object` with `register(...)`.
2. Moved `metricRegistry` to the first parameter.
3. Updated the call site in `LivyServer.scala` accordingly.
All 8 tests in `LivySessionMetricsSpec` pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
Posted by "gyogal (via GitHub)" <gi...@apache.org>.
gyogal commented on code in PR #543:
URL: https://github.com/apache/livy/pull/543#discussion_r3804319719
##########
server/src/main/scala/org/apache/livy/server/LivyServer.scala:
##########
@@ -252,6 +252,8 @@ class LivyServer extends Logging {
}
context.mountMetricsAdminServlet("/metrics")
+ new LivySessionMetrics(
+ interactiveSessionManager, batchSessionManager, metricRegistry)
Review Comment:
The choice of syntax seems a bit unusual here, because (if I understand correctly) what ends up happening is `metricRegistry.register()` gets called multiple times, and the resulting `LivySessionMetrics` reference is not stored anywhere. Is this the convention with Codehale gauge setup, or why is `LivySessionMetrics` not an object for example and why is `metricRegistry` the last argument, even though that is what is mostly being operated on? I think those changes could make the code clearer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
Posted by "samuhasavak5 (via GitHub)" <gi...@apache.org>.
samuhasavak5 commented on PR #543:
URL: https://github.com/apache/livy/pull/543#issuecomment-5396450851
> @samuhasavak5
>
> Thanks for the PR! Tested it locally and was able to query the endpoint successfully at http://localhost:8998/metrics/metrics?pretty=true.
>
> Could you also update `docs/rest-api.md` to cover the new metrics? It looks like the `/metrics` endpoint and the new gauge names aren't documented yet.
Hi @roczei
Thank you for testing and for the feedback.
I have updated `docs/rest-api.md` to document the `/metrics/metrics` endpoint
and the 18 `livy.sessions.*` session gauges (commit f6cdc958).
Please take another look when you have a moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [LIVY-1070] Add LivySessionMetrics Codahale gauges for session monitoring [livy]
Posted by "samuhasavak5 (via GitHub)" <gi...@apache.org>.
samuhasavak5 commented on PR #543:
URL: https://github.com/apache/livy/pull/543#issuecomment-5294738067
@roczei , @gyogal , @nileshrathi345 and @ArnavBalyan
could you please take a look when possible and review
Thanks..!!!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@livy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org