HADOOP-17804. Expose prometheus metrics only after a flush and dedupe with tag values #3369
Conversation
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
|
💔 -1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
feature/prometheus-metrics-fix
branch
from
8e80be8 to
9bee65b
Compare
|
🎊 +1 overall
This message was automatically generated. |
Member
|
@Kimahriman |
Contributor
|
LGTM + 1. |
Contributor
Author
Ahh didn't see that one, should be fixed now. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Member
|
@Kimahriman Thank you for fixing the checkstyle issues. Would you fix trailing whitespace? I'm +1 if that is addressed. |
Kimahriman
force-pushed
the
feature/prometheus-metrics-fix
branch
from
ea51dbb to
255223c
Compare
Contributor
Author
|
Clearly don't have my dev env setup to tell me these things yet. Third try is the charm |
|
🎊 +1 overall
This message was automatically generated. |
aajisaka
requested changes
Sep 8, 2021
...oject/hadoop-common/src/main/java/org/apache/hadoop/metrics2/sink/PrometheusMetricsSink.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Seems to be a race condition in the test itself, investigating... |
|
🎊 +1 overall
This message was automatically generated. |
aajisaka
approved these changes
Sep 9, 2021
Member
aajisaka
left a comment
aajisaka
left a comment
There was a problem hiding this comment.
+1. Thank you for the update.
aajisaka
pushed a commit
that referenced
this pull request
Sep 9, 2021
… with tag values (#3369) Signed-off-by: Akira Ajisaka <[email protected]> (cherry picked from commit 4ced012)
kiran-maturi
pushed a commit
to kiran-maturi/hadoop
that referenced
this pull request
Nov 24, 2021
… with tag values (apache#3369) Signed-off-by: Akira Ajisaka <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Description of PR
Fixes a bug with the Prometheus metrics sink where metrics were deduped on their name alone, and didn't include the tag values for deduplication purposes. Prometheus metrics are uniquely identified by their name and labels, so several metrics were just getting dropped. Specifically things like RPC metrics were only including one of the servers/ports per metric type, and Yarn queue metrics only included metrics for one queue.
Additionally, because of the "push" nature of Hadoop metrics, this would end up creating a lot of extra metrics for things where the tags can change over time but they still actually mean the same thing. For example, the
hastateof namenode metrics can change, but you really only want the most recent one. To address this, I changed it to only expose metrics after aflushcall, and to start fresh after eachflushcall. This prevents old metrics from hanging around and constantly being exposed until the service is restarted.There are still some "bad" tags that are exposed which can lead to multiple Prometheus series being created when really they are the same thing. However, these can be dealt with on the Prometheus side, ignoring certain labels, rather than trying to hard code all the bad tags on the Hadoop side.
I don't think there should be any threading/race conditions with publishing metrics, since the publish metrics methods are synchronized.
Also adds the help line to the output.
How was this patch tested?
New unit tests.
For code changes:
Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?If applicable, have you updated theLICENSE,LICENSE-binary,NOTICE-binaryfiles?