-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-32026][CORE][TEST] Add PrometheusServletSuite #28865
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
[SPARK-32026][CORE][TEST] Add PrometheusServletSuite #28865
Conversation
|
Test build #124244 has finished for PR 28865 at commit
|
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.
Thank you for contribution, @erenavsarogullari .
However, Prometheus Servlet is designed to be consistent with the existing Spark 2.4 JMX Sink + Prometheus JMX Exporter format. Also, inside Spark 3.0, the format will follow Spark 3.0 JMX Sink + Prometheus JMX Exporter style mostly. Could you compare your output result with JMX Sink + Prometheus Exporter result of Spark 3.0 first?
For the metric name, we know that's not proper Prometheus. So, we recommend to use spark.metrics.namespace and writeStream.queryName().
It's described in original JIRAs, but the following presentation slides for Spark+AI Summit 2020 will be helpful for you if you are interested in this area.
|
Hi @dongjoon-hyun, Thanks for the review and details. Prometheus format seems following JMX-Exporter and also introducing type attribute(e.g: type="gauges"). I think we would like to continue preserving the same format, right? On the other hand, what about Unit Tests for Jmx: Prometheus: |
|
No. Please test on Apache Spark 3.0.0. DropWizard 4 itself introduced type attribute(e.g: type="gauges") and
|
core/src/test/scala/org/apache/spark/metrics/sink/PrometheusServletSuite.scala
Show resolved
Hide resolved
dongjoon-hyun
left a comment
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.
Let's focus on adding a unit test, @erenavsarogullari . Also, please update JIRA and PR title~ Thank you for your contribution, @erenavsarogullari .
|
Retest this please |
core/src/test/scala/org/apache/spark/metrics/sink/PrometheusServletSuite.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/apache/spark/metrics/sink/PrometheusServletSuite.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/apache/spark/metrics/sink/PrometheusServletSuite.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/apache/spark/metrics/sink/PrometheusServletSuite.scala
Outdated
Show resolved
Hide resolved
|
Also, please update the PR description consistently together. Thanks! |
|
Retest this please. |
|
Thanks @dongjoon-hyun for the review. All comments are addressed. I think it is ready to go. |
|
Test build #124665 has finished for PR 28865 at commit
|
|
Sure, you can do that in the custom release.
|
core/src/test/scala/org/apache/spark/metrics/sink/PrometheusServletSuite.scala
Outdated
Show resolved
Hide resolved
|
Test build #124689 has finished for PR 28865 at commit
|
|
Retest this please |
|
Test build #124698 has finished for PR 28865 at commit
|
|
Retest this please |
|
Test build #124802 has finished for PR 28865 at commit
|
|
Failure seems irrelevant |
|
Ya. I'll test it manually, @erenavsarogullari . Thanks for updating. |
dongjoon-hyun
left a comment
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.
+1, LGTM. Thank you, @erenavsarogullari .
I tested this new test suite locally. Merged to master branch for Apache Spark 3.1 on December 2020.
|
Thanks @dongjoon-hyun for follow-up and merge. |
What changes were proposed in this pull request?
This PR aims to add
PrometheusServletSuite.Why are the changes needed?
This improves the test coverage.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass the newly added test suite.