Uh oh!
There was an error while loading. Please reload this page.
HBASE-20904 Prometheus /metrics http endpoint for monitoring - #4691
Merged
Conversation
Co-authored-by: Luca Kovacs <kovacs.luca.agota@gmail.com>
Apache-HBase
commented
Aug 10, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 10, 2022
| Collection<MetricsRecord> metricRecords = MetricsExportHelper.export(); | ||
| for (MetricsRecord metricsRecord : metricRecords) { | ||
| for (AbstractMetric metrics : metricsRecord.metrics()) { | ||
| if (metrics.type() == MetricType.COUNTER || metrics.type() == MetricType.GAUGE) { |
Contributor
There was a problem hiding this comment.
We do not want to rely on prometheus simpleclient to do this?
ContributorAuthor
There was a problem hiding this comment.
It was part of the original pull request and because it doesn't add any dependencies I kept it like this. If you think simpleclient is a better solution, it can be replaced of course.
Contributor
There was a problem hiding this comment.
No problem, just want to know the reason.
Then better add some references about the format spec, and also mention that the format is not hard to implement so do not want to add extra dependencies.
| import org.apache.yetus.audience.InterfaceAudience; | ||
| @InterfaceAudience.Private | ||
| public class PrometheusHadoop2Servlet extends HttpServlet { |
Contributor
There was a problem hiding this comment.
Why name it hadoop2, not hadoop?
ContributorAuthor
There was a problem hiding this comment.
First, I left it as it was, but I renamed to Hadoop in the last commit
Apache-HBase
commented
Aug 11, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 11, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 11, 2022
💔 -1 overall
This message was automatically generated. |
Notes: - Fixed error that caused test fails - Renamed PrometheusHadoop2Servlet to PrometheusHadoopServlet (same in the test files) - Added comments, change in the documenatation
Apache-HBase
commented
Aug 15, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 15, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 16, 2022
Uh oh!
There was an error while loading. Please reload this page.
Notes: - Fixed RestrictedAPI annontation - Added '/metrics' endpoint to defaults
Apache-HBase
commented
Aug 16, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 16, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 17, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 17, 2022
Apache9
left a comment
Contributor
There was a problem hiding this comment.
No big problem. Just some style nits.
| public static Collection<MetricsRecord> export() { | ||
| MetricsSystemImpl instance = (MetricsSystemImpl) DefaultMetricsSystem.instance(); | ||
| MetricsBuffer metricsBuffer = instance.sampleMetrics(); | ||
| List<MetricsRecord> metrics = new LinkedList<>(); |
Contributor
There was a problem hiding this comment.
Use ArrayList. LinkedList will lead to a error prone warning.
| public static final String METRIC_SERVLETS_CONF_KEY = "hbase.http.metrics.servlets"; | ||
| public static final String METRICS_SERVLETS_DEFAULT[] = { "jmx", "metrics", "prometheus" }; | ||
| private static final Map<String, ServletConfig> METRIC_SERVLETS = | ||
| new HashMap<String, ServletConfig>() { |
Contributor
There was a problem hiding this comment.
Use ImmutableMap.Builder in guava. In hbase we have a shaded guava, under the org.apache.hbase.thirdparty.com.google package. The current code will lead to a error prone warning.
| import org.apache.yetus.audience.InterfaceAudience; | ||
| @InterfaceAudience.Private | ||
| public class MetricsExportHelper { |
Contributor
There was a problem hiding this comment.
Make this class final and add a private constructor.
| String metricName) { | ||
| return metrics.stream().filter(mr -> mr.name().equals(metricsName)).anyMatch(mr -> { | ||
| for (AbstractMetric metric : mr.metrics()) { | ||
| if (metric.name().equals(metricName)) return true; |
Contributor
There was a problem hiding this comment.
New line with '{}', otherwise there will be a checkstyle warning.
| public static final String APP_DIR = "webapps"; | ||
| public static final String METRIC_SERVLETS_CONF_KEY = "hbase.http.metrics.servlets"; | ||
| public static final String METRICS_SERVLETS_DEFAULT[] = { "jmx", "metrics", "prometheus" }; |
Contributor
There was a problem hiding this comment.
String METRICS_SERVLETS_DEFAULT[] -> String[] METRICS_SERVLETS_DEFAULT
| } | ||
| /* register metrics servlets */ | ||
| String enabledServlets[] = conf.getStrings(METRIC_SERVLETS_CONF_KEY, METRICS_SERVLETS_DEFAULT); |
Contributor
There was a problem hiding this comment.
String enabledServlets[] -> String[] enabledServlets
lucakovacs
commented
Aug 17, 2022
ContributorAuthor
Fixed them :) |
Apache-HBase
commented
Aug 17, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 18, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 18, 2022
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Aug 20, 2022
Contributor
Please take a look at the findbugs issue? |
Apache-HBase
commented
Aug 23, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 23, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 23, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
approved these changes
Aug 24, 2022
Apache9 pushed a commit
that referenced
this pull request
Aug 24, 2022
Co-authored-by: Luca Kovacs <kovacs.luca.agota@gmail.com> Co-authored-by: Madhusoodan P <akshayapataki123@gmail.com> Co-authored-by: Luca Kovacs <lkovacs@cloudera.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit f9ea7ee) Conflicts: src/main/asciidoc/_chapters/ops_mgt.adoc
ndimiduk
left a comment
Member
There was a problem hiding this comment.
Hi there @lucakovacs , thanks for the contribution.
I must confess, I'm surprised that this project was resumed and committed. The last discussion on it was in 2020 when I expressed my reservations about implementing it. I think that we should not implement any code ourselves and instead use something like https://github.com/prometheus/jmx_exporter.
All that said, I have a couple concerns with the implementation as it is. Please take a look. If the community still wants this implementation instead of depending on a 3rd party agent, and if my questions can be addressed, we can probably include this one in the next 2.5.0 release candidate.
Thanks!
| clz.asSubclass(HttpServlet.class)); | ||
| } | ||
| } catch (Exception e) { | ||
| /* shouldn't be fatal, so warn the user about it */ |
Member
There was a problem hiding this comment.
I'm surprised that having metrics be optional was accepted by review. Before this change, it was impossible to run an HBase processes that exposes no metrics at all. Now, that can happen. We do not have other healthcheck endpoints like a modern web service, metrics are the only lifeline for an operations team (besides inspecting the pid). I think that we should be more careful here:
- we should warn if the process is configured without any metrics endpoint
- we should warn if a configured metric endpoint fails to load
- we should abort the process launch if the process is configured with any metrics but none load
Contributor
There was a problem hiding this comment.
For me, I think the current implementation is enough. We will warn if a registered endpoint fails to load. And the default configuration is to load all metrics endpoints, if users configured it to none, they must have a reason, so I do not think here we need to warn it. And I also do not think we should abort the process if none metrics can be loaded, as it does not affect the normal read/write.
| import org.apache.yetus.audience.InterfaceAudience; | ||
| @InterfaceAudience.Private | ||
| public class PrometheusHadoopServlet extends HttpServlet { |
Member
There was a problem hiding this comment.
Why is this called a "Hadoop" servlet?
Contributor
There was a problem hiding this comment.
Because it exports the metrics we registered to hadoop metrics system?
| import org.apache.yetus.audience.InterfaceAudience; | ||
| @InterfaceAudience.Private | ||
| public class PrometheusHadoopServlet extends HttpServlet { |
Member
There was a problem hiding this comment.
Oh no, why was this implemented as a raw servlet :'(
We have Jersey. There is absolutely no reason to implement a servlet by hand.
Contributor
There was a problem hiding this comment.
I do not see any advantages here to use jersey? The servlet is really easy to implement...
| new ServletConfig("jmx", "/jmx", "org.apache.hadoop.hbase.http.jmx.JMXJsonServlet")) | ||
| .put("metrics", | ||
| new ServletConfig("metrics", "/metrics", "org.apache.hadoop.metrics.MetricsServlet")) | ||
| .put("prometheus", new ServletConfig("prometheus", "/prometheus", |
Member
There was a problem hiding this comment.
We should have changed the title of the jira to match the actual endpoint this feature uses. People will be confused that /metrics does not emit the prometheus formatted output.
Contributor
There was a problem hiding this comment.
Ah, this is a problem. I just read it as 'Prometheus metrics http endpoint for monitoring'... Missed the '/'
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…4691) Co-authored-by: Luca Kovacs <kovacs.luca.agota@gmail.com> Co-authored-by: Madhusoodan P <akshayapataki123@gmail.com> Co-authored-by: Luca Kovacs <lkovacs@cloudera.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit f9ea7ee) Change-Id: I31ec9c68ffd05c0dc42acb8f4462d1e61c4741cf
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Co-authored-by: Luca Kovacs kovacs.luca.agota@gmail.com