HADOOP-18167. Add metrics to track delegation token secret manager op… #4092
Conversation
This file contains 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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
simbadzina
reviewed
Mar 23, 2022
Comment on lines
455
to
457
| if (metrics != null) { | ||
| metrics.addStoreToken(Time.monotonicNow() - start); | ||
| } |
There was a problem hiding this comment.
A metric will still be recorded when storeToken fails. Is this the same pattern used for other operations?
There was a problem hiding this comment.
This can help us correlate how changes in latency affect the whole operation. For example, if calling storeToken times out after x seconds, we should be able to recognize it by looking at this metric. That's also the pattern that I see with RpcMetrics
There was a problem hiding this comment.
I'd be happier if the addStoreToken was only updated when it was successful. We should track failures separately with a counter. (I don't think we need to track the operation that had the exception, just track the count of exceptions that happened getting delegation tokens.
There was a problem hiding this comment.
I've made changes to only increment the storeToken, updateToken and removeToken metrics when the operation was successful. I also added a new counter "tokenFailure" that is updated when the operation is not successful.
| long start = Time.monotonicNow(); | ||
| try { | ||
| updateToken(id, info); | ||
| } finally { |
There was a problem hiding this comment.
Same issue as above, emitting metrics when there is a failure.
| final static String UPDATE_TOKEN_STAT = "updateToken"; | ||
| final static String REMOVE_TOKEN_STAT = "removeToken"; | ||
|
|
||
| final MetricsRegistry registry = new MetricsRegistry("DelegationTokenSecretManagerMetrics"); |
There was a problem hiding this comment.
I'm mainly adding it now to reduce the risk of someone changing the registry name. If we don't add this, metrics will be registered with the class name ("DelegationTokenSecretManagerMetrics"). However, if someone decides to add the registry in the future, they could choose a different name and break something. I prefer to just have it available if someone ever needs it. I've seen the same pattern in other metrics classes (ClientSCMMetrics, SharedCacheUploaderMetrics,etc), but I'm open to change this if you see a bigger problem.
There was a problem hiding this comment.
I see. A minor change would be to add "LOG.debug("Initialized {}", registry);" like in ClientSCMMetrics just so the IDE and potential our style check steps in testing don't fail.
The creation of the registry can also be part of the constructor.
omalley
requested changes
Mar 24, 2022
Comment on lines
455
to
457
| if (metrics != null) { | ||
| metrics.addStoreToken(Time.monotonicNow() - start); | ||
| } |
There was a problem hiding this comment.
I'd be happier if the addStoreToken was only updated when it was successful. We should track failures separately with a counter. (I don't think we need to track the operation that had the exception, just track the count of exceptions that happened getting delegation tokens.
|
🎊 +1 overall
This message was automatically generated. |
omalley
requested changes
Mar 28, 2022
| } | ||
|
|
||
| public void addStoreToken(boolean success, long value) { | ||
| if (success) { |
There was a problem hiding this comment.
Rather than have three methods add the success with separate code paths for true/false. It would be nicer to have a single addFailure.
There was a problem hiding this comment.
I've added method addTokenFailure so we can call it separately
| } catch (IOException ioe) { | ||
| LOG.error("Could not store token " + formatTokenId(identifier) + "!!", | ||
| ioe); | ||
| } finally { |
There was a problem hiding this comment.
The problem with try finally, is that if the code throws an exception after an exception in the body, the original exception is lost, which makes it very hard to debug.
Please add the positive call into the body and the failure count in the ioexception handler. It will mean that we don't count the runtime exceptions, but I think that is ok in this context.
There was a problem hiding this comment.
I've move the call to track metrics in the body and the exception handler. If the code in the exception handler throws another exception we would still lose the original exception though.
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Apr 14, 2022
| @@ -50,6 +59,9 @@ | |||
| import org.slf4j.Logger; | |||
| import org.slf4j.LoggerFactory; | |||
|
|
|||
| import static org.apache.hadoop.fs.statistics.impl.IOStatisticsBinding.*; | |||
There was a problem hiding this comment.
Better to expand this or to use the full name.
There was a problem hiding this comment.
I've expanded this
| LOG.debug("Initialized {}", registry); | ||
| } | ||
|
|
||
| public void addStoreToken(long value) { |
There was a problem hiding this comment.
And probably time and the unit instead of plain "value"
There was a problem hiding this comment.
Thanks. I've updated method names, argument name and made it final
| ioStatistics.addTimedOperation(STORE_TOKEN_STAT, value); | ||
| } | ||
|
|
||
| public void addUpdateToken(long value) { |
There was a problem hiding this comment.
addTimeUpdateToken() and same for the rest.
| public void testDelegationTokenSecretManagerMetrics() throws Exception { | ||
| TestDelegationTokenSecretManager dtSecretManager = | ||
| new TestDelegationTokenSecretManager(24*60*60*1000, | ||
| 10*1000,1*1000,3600000); |
|
|
||
| try { | ||
| dtSecretManager.cancelToken(token, "JobTracker"); | ||
| Assert.fail("Expected exception"); |
There was a problem hiding this comment.
This is very helpful. Thanks!
goiri
approved these changes
Apr 16, 2022
| } | ||
|
|
||
| public DelegationTokenSecretManagerMetrics() { | ||
| ioStatistics = iostatisticsStore() |
There was a problem hiding this comment.
For this specific case, I would skip the static import and just do:
IOStatisticsBinding.iostatisticsStore()
There was a problem hiding this comment.
Removed static import
| generateDelegationToken(dtSecretManager, "SomeUser", "JobTracker"); | ||
| Assert.assertEquals(1, dtSecretManager.metrics.tokenFailure.value()); | ||
|
|
||
| LambdaTestUtils.intercept(Exception.class, () -> dtSecretManager.renewToken(token, "JobTracker")); |
There was a problem hiding this comment.
It would be nice to be a little more specific than just Exception
There was a problem hiding this comment.
also, if on failure that renew call sleeps for a few hundred millis, you could assert that the duration of the failure was measured/counted. IOStatisticAssertions helps there
There was a problem hiding this comment.
I've updated this to expect IOException, since that's the exception we throw in TestFailureDelegationTokenSecretManager. We're currently not tracking duration of failures per a previous comment by @omalley
steveloughran
requested changes
Apr 16, 2022
| @@ -96,6 +108,10 @@ private String formatTokenId(TokenIdent id) { | |||
| * Access to currentKey is protected by this object lock | |||
| */ | |||
| private DelegationKey currentKey; | |||
| /** | |||
| * Metrics to track token management operations | |||
There was a problem hiding this comment.
nit: add a trailing .
| @@ -430,10 +447,13 @@ protected synchronized byte[] createPassword(TokenIdent identifier) { | |||
| DelegationTokenInformation tokenInfo = new DelegationTokenInformation(now | |||
| + tokenRenewInterval, password, getTrackingIdIfEnabled(identifier)); | |||
| try { | |||
| long start = Time.monotonicNow(); | |||
There was a problem hiding this comment.
IOStatisticsStore is a DurationTrackerFactory; you can use it in IOStatisticsBinding invocations to have it track min/mean/max duration of ops. distinguishing success and failure times
| * and publishes them through the metrics interfaces. | ||
| */ | ||
| @Metrics(about="Delegation token secret manager metrics", context="token") | ||
| static class DelegationTokenSecretManagerMetrics implements IOStatisticsSource { |
There was a problem hiding this comment.
make this a DurationTrackerFactory and you can supply duration trackers to IOStatisticsBinding; note also there's a PairedDurationTrackerFactory which you could wire up the iostats store. maybe this would be the time/place to add a DurationTrackerFactory for updating hadoop metrics
There was a problem hiding this comment.
Can we make the DurationTrackerFactory changes on a separate PR? We're might not need to track failure durations here
| try { | ||
| dtSecretManager.startThreads(); | ||
|
|
||
| Assert.assertEquals(0, dtSecretManager.metrics.storeToken.lastStat().numSamples()); |
There was a problem hiding this comment.
this is invoked enough it is worth factoring into its own assertion
There was a problem hiding this comment.
Refactored to group the assertions and the invocation
| generateDelegationToken(dtSecretManager, "SomeUser", "JobTracker"); | ||
| Assert.assertEquals(1, dtSecretManager.metrics.tokenFailure.value()); | ||
|
|
||
| LambdaTestUtils.intercept(Exception.class, () -> dtSecretManager.renewToken(token, "JobTracker")); |
There was a problem hiding this comment.
also, if on failure that renew call sleeps for a few hundred millis, you could assert that the duration of the failure was measured/counted. IOStatisticAssertions helps there
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Apr 20, 2022
| */ | ||
| @Metrics(about="Delegation token secret manager metrics", context="token") | ||
| static class DelegationTokenSecretManagerMetrics implements IOStatisticsSource { | ||
| private static final Logger LOG = LoggerFactory.getLogger(DelegationTokenSecretManagerMetrics.class); |
There was a problem hiding this comment.
This is giving check style issues from Yetus.
| final IOStatisticsStore ioStatistics; | ||
|
|
||
| @Metric("Rate of storage of delegation tokens and latency (milliseconds)") | ||
| MutableRate storeToken; |
There was a problem hiding this comment.
This is giving warnings in checkstyle but I believe is needed right?
There was a problem hiding this comment.
I've made this private and added accessor methods
...op-common/src/test/java/org/apache/hadoop/security/token/delegation/TestDelegationToken.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
goiri
approved these changes
Apr 21, 2022
| Assert.assertEquals(expectedCount - 1, metric.lastStat().numSamples()); | ||
| Assert.assertEquals(expectedCount - 1, stat.getSamples()); | ||
| T returnedObject = callable.call(); | ||
| Assert.assertEquals(expectedCount, metric.lastStat().numSamples()); |
There was a problem hiding this comment.
We are doing a static import of assertEquals, so no need to do Assert.assertEquals() and assertEquals() is enough.
There was a problem hiding this comment.
I've updated to use assertEquals() only
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
omalley
pushed a commit
that referenced
this pull request
Apr 26, 2022
#4092) * HADOOP-18167. Add metrics to track delegation token secret manager operations
|
@hchaverri @omalley I found that your submission in HADOOP-18167 may cause an online bug in Yarn. The code you submitted will cause the DelegationTokenSecretManagerMetrics to be initialized twice in Yarn ResourceManager and throw an exception. Please check YARN-11123 |
|
Thanks @slfan1989 . There was another report earlier and created HADOOP-18222. A fix is being reviewed on #4266 |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
apache#4092) * HADOOP-18167. Add metrics to track delegation token secret manager operations
NyteKnight
pushed a commit
to NyteKnight/hadoop
that referenced
this pull request
Jun 25, 2024
apache#4092) * HADOOP-18167. Add metrics to track delegation token secret manager operations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…erations
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?