CASSSIDECAR-216: Capture Metrics for Schema Reporting #204
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
5
force-pushed
the
schema-reporting-metrics
branch
from
e1c05cf to
64a48a3
Compare
5
changed the title
yifan-c
reviewed
Mar 7, 2025
Comment on lines
+254
to
+262
| return new ToStringBuilder(this, STYLE) | ||
| .append(this.organization()) | ||
| .append(this.platform()) | ||
| .append(this.environment()) | ||
| .append(this.application()) | ||
| .append(this.cluster()) | ||
| .append(this.identifier()) | ||
| .toString(); | ||
| .toString() | ||
| .replaceAll("\\s", ""); |
There was a problem hiding this comment.
The toString is relatively complex. Is there a test?
| @NotNull DeltaGauge started) | ||
| { | ||
| String action = " reporting schema for cluster with identifiers " + identifiersProvider; | ||
| LOGGER.info("Started" + action); |
There was a problem hiding this comment.
Use the value placeholder of logger to eval lazily. Update the other places too.
Suggested change
| LOGGER.info("Started" + action); | |
| LOGGER.info("Started {}", action); |
| private void process(@NotNull Metadata metadata, | ||
| @NotNull DeltaGauge started) | ||
| { | ||
| String action = " reporting schema for cluster with identifiers " + identifiersProvider; |
There was a problem hiding this comment.
you can remove the heading space. It should make the error message in the RuntimeException look a bit nicer too.
Comment on lines
+33
to
+53
| protected static final String DOMAIN = ServerMetrics.SERVER_PREFIX + ".SchemaReporting"; | ||
| protected static final String STARTED = "Started"; | ||
| protected static final String FINISHED = "Finished"; | ||
| protected static final String DURATION = "Duration"; | ||
| protected static final String SIZE = "Size"; | ||
| protected static final String TRIGGER = "Trigger"; | ||
| protected static final String RESULT = "Result"; | ||
| protected static final String UNIT = "Unit"; | ||
| protected static final String REQUEST = "Request"; | ||
| protected static final String SCHEDULE = "Schedule"; | ||
| protected static final String SUCCESS = "Success"; | ||
| protected static final String FAILURE = "Failure"; | ||
| protected static final String ASPECTS = "Aspects"; | ||
| protected static final String MILLISECONDS = "Milliseconds"; | ||
|
|
||
| public final NamedMetric<DeltaGauge> startedRequest; | ||
| public final NamedMetric<DeltaGauge> startedSchedule; | ||
| public final NamedMetric<DeltaGauge> finishedSuccess; | ||
| public final NamedMetric<DeltaGauge> finishedFailure; | ||
| public final NamedMetric<Histogram> durationMilliseconds; | ||
| public final NamedMetric<Histogram> sizeAspects; |
There was a problem hiding this comment.
Please update the patch to follow the other Metrics class? Let them be consistent. The string constants that are only used once can all be removed.
Comment on lines
+57
to
+89
| startedSchedule = NamedMetric.builder(name -> registry.gauge(name, DeltaGauge::new)) | ||
| .withDomain(DOMAIN) | ||
| .withName(STARTED) | ||
| .addTag(TRIGGER, SCHEDULE) | ||
| .build(); | ||
| startedRequest = NamedMetric.builder(name -> registry.gauge(name, DeltaGauge::new)) | ||
| .withDomain(DOMAIN) | ||
| .withName(STARTED) | ||
| .addTag(TRIGGER, REQUEST) | ||
| .build(); | ||
|
|
||
| finishedSuccess = NamedMetric.builder(name -> registry.gauge(name, DeltaGauge::new)) | ||
| .withDomain(DOMAIN) | ||
| .withName(FINISHED) | ||
| .addTag(RESULT, SUCCESS) | ||
| .build(); | ||
| finishedFailure = NamedMetric.builder(name -> registry.gauge(name, DeltaGauge::new)) | ||
| .withDomain(DOMAIN) | ||
| .withName(FINISHED) | ||
| .addTag(RESULT, FAILURE) | ||
| .build(); | ||
|
|
||
| sizeAspects = NamedMetric.builder(registry::histogram) | ||
| .withDomain(DOMAIN) | ||
| .withName(SIZE) | ||
| .addTag(UNIT, ASPECTS) | ||
| .build(); | ||
|
|
||
| durationMilliseconds = NamedMetric.builder(registry::histogram) | ||
| .withDomain(DOMAIN) | ||
| .withName(DURATION) | ||
| .addTag(UNIT, MILLISECONDS) | ||
| .build(); |
There was a problem hiding this comment.
Prefer self-describing names over tags. For example, "StartedBySchedule" provides better readability than "Trigger=Schedule.Started". Note that all the tags are combined into this key=value format at the end.
Comment on lines
+145
to
+152
| assertEquals(0L, metrics.startedRequest.metric.getValue()); | ||
| assertEquals(1L, metrics.startedSchedule.metric.getValue()); | ||
| assertEquals(1L, metrics.finishedSuccess.metric.getValue()); | ||
| assertEquals(0L, metrics.finishedFailure.metric.getValue()); | ||
| assertEquals(1L, metrics.sizeAspects.metric.getCount()); | ||
| assertEquals(13L, metrics.sizeAspects.metric.getSnapshot().getValues()[0]); | ||
| assertEquals(1L, metrics.durationMilliseconds.metric.getCount()); | ||
| assertTrue(0L <= metrics.durationMilliseconds.metric.getSnapshot().getValues()[0]); |
Comment on lines
+152
to
+159
| assertEquals(1L, metrics.startedRequest.metric.getValue()); // * one execution triggered by request | ||
| assertEquals(0L, metrics.startedSchedule.metric.getValue()); // * zero executions triggered by schedule | ||
| assertEquals(1L, metrics.finishedSuccess.metric.getValue()); // * one execution resulted in success | ||
| assertEquals(0L, metrics.finishedFailure.metric.getValue()); // * zero executions resulted in failure | ||
| assertEquals(1L, metrics.sizeAspects.metric.getCount()); // * single number of aspects, | ||
| assertEquals(13L, metrics.sizeAspects.metric.getSnapshot().getValues()[0]); // equal to thirteen | ||
| assertEquals(1L, metrics.durationMilliseconds.metric.getCount()); // * single duration of execution, | ||
| assertTrue(0L <= metrics.durationMilliseconds.metric.getSnapshot().getValues()[0]); // that is non-negative |
Comment on lines
+321
to
+329
| SchemaReportingMetrics metrics = this.metrics.server().schemaReporting(); // Validate captured metrics: | ||
| assertEquals(0L, metrics.startedRequest.metric.getValue()); // * zero executions triggered by request | ||
| assertEquals(1L, metrics.startedSchedule.metric.getValue()); // * one execution triggered by schedule | ||
| assertEquals(1L, metrics.finishedSuccess.metric.getValue()); // * one execution resulted in success | ||
| assertEquals(0L, metrics.finishedFailure.metric.getValue()); // * zero executions resulted in failure | ||
| assertEquals(1L, metrics.sizeAspects.metric.getCount()); // * single number of aspects, | ||
| assertEquals(13L, metrics.sizeAspects.metric.getSnapshot().getValues()[0]); // equal to thirteen | ||
| assertEquals(1L, metrics.durationMilliseconds.metric.getCount()); // * single duration of execution, | ||
| assertTrue(0L <= metrics.durationMilliseconds.metric.getSnapshot().getValues()[0]); // that is non-negative |
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.
(Only the second commit needs to be reviewed, as the first one is being handled separately in #198)