Metrics Refactoring #118
Metrics Refactoring #118
Conversation
| } | ||
| | ||
| /** | ||
| * Test that a {@link KeyBuilder} with parameters have them properly formatted. |
There was a problem hiding this comment.
just curious, what happens if you do
final String metricName = "custom_metric.{}.{}"; .... keyBuilder.build(metricDefinition, new String[]{ param1 })?
There was a problem hiding this comment.
I think the second set of {} just hangs there. String replacement is handled by the same formatted used for logging.
There was a problem hiding this comment.
@Crim I went ahead and added a test to confirm my hypothesis.
| public void close() { | ||
| // Noop | ||
| } | ||
| private final KeyBuilder keyBuilder = new KeyBuilder(null); |
There was a problem hiding this comment.
Should this be set in open()? Is KeyBuilder serializable?
There was a problem hiding this comment.
Hmmm... good call. It probably serializes fine, but this is a good reason to put it in open(), though I hope no one uses this recorder in production.
| * | ||
| * @param clock clock instance for testing | ||
| */ | ||
| void setClock(final Clock clock) { |
There was a problem hiding this comment.
Wonder if this should be a package protected constructor?
There was a problem hiding this comment.
Tried that, but it didn’t work with the test since I need to pass in a new clock on the same instance. Check out the start stop test.
| * @param key key for timer | ||
| */ | ||
| void start(final String key) { | ||
| start(key, getClock().millis()); |
There was a problem hiding this comment.
On the clock? I’ll probably add that to the getter. Good call.
There was a problem hiding this comment.
Added some null checks for both key and clock as well as covering tests.
Crim left a comment
Crim left a comment There was a problem hiding this comment.
probably need to fix LogRecorder comment, but looks good so far
| seems legit |
First part in my work on #117, this takes several bits of code that are reused between
StormRecorderandLogRecorderand breaks them out so that they can be used in a forthcomingDropwizardRecorderimplementation.