[MIGRATIONS-1326] Add status code comparison metric #430
Conversation
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Signed-off-by: Mikayla Thompson <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #430 +/- ##
============================================
- Coverage 63.16% 62.98% -0.19%
Complexity 873 873
============================================
Files 100 100
Lines 4341 4355 +14
Branches 410 411 +1
============================================
+ Hits 2742 2743 +1
- Misses 1328 1340 +12
- Partials 271 272 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mikayla Thompson <[email protected]>
mikaylathompson
requested review from
chelma,
gregschohn,
kartg,
lewijacn,
okhasawn and
sumobrian
as code owners
| @@ -76,7 +76,6 @@ public MetricsLogBuilder atError(MetricsEvent event, Throwable cause) { | |||
| } | |||
| return new MetricsLogBuilder(logger).atError(event) | |||
| .setAttribute(MetricsAttributeKey.EXCEPTION_MESSAGE, cause.getMessage()) | |||
| .setAttribute(MetricsAttributeKey.EXCEPTION_STACKTRACE, cause.getStackTrace().toString()) | |||
There was a problem hiding this comment.
For future reference, this change is NOT required to add a new metric.
| String sourceResponseStatus = (String) meta.getJSONObject("sourceResponse").get("Status-Code"); | ||
| String targetResponseStatus = (String) meta.getJSONObject("targetResponse").get("Status-Code"); |
There was a problem hiding this comment.
The sourceResponse/targetResponse are filled in with code that runs through this function...
private JSONObject jsonFromHttpData(@NonNull List<byte[]> data) {
try {
return jsonFromHttpDataUnsafe(data);
} catch (Exception e) {
log.warn("Putting what may be a bogus value in the output because transforming it " +
"into json threw an exception");
return new JSONObject(Map.of("Exception", e.toString()));
}
}
That means that we might have the "Status-Code" values of null. That will be an issue down at line 137 where we'll throw an NPE.
| @@ -119,6 +125,26 @@ private JSONObject toJSONObject(SourceTargetCaptureTuple tuple) { | |||
| .ifPresent(d-> meta.put("targetResponse", jsonFromHttpData(d, tuple.targetResponseDuration))); | |||
| meta.put("connectionId", tuple.uniqueRequestKey); | |||
| Optional.ofNullable(tuple.errorCause).ifPresent(e->meta.put("error", e)); | |||
|
|
|||
| if (meta.has("sourceResponse") && meta.has("targetResponse")) { | |||
There was a problem hiding this comment.
If there was no response for the target (or even the source), we probably want to still emit the other status code, and especially the part that it doesn't match, right?
| @@ -27,6 +31,8 @@ | |||
| @Slf4j | |||
| public class SourceTargetCaptureTuple implements AutoCloseable { | |||
| public static final String OUTPUT_TUPLE_JSON_LOGGER = "OutputTupleJsonLogger"; | |||
| private static final MetricsLogger metricsLogger = new MetricsLogger("SourceTargetCaptureTuple"); | |||
There was a problem hiding this comment.
It would be a lot cleaner if this were decomposed into separate responsibilities. Here's what we do with the raw requests/responses.
Some of those handlers may convert these to higher order messages.
When we have higher order messages, we should be able to attach consumers/listeners on top of those.
Right now, it's exceptionally easy to see how metrics would be completely suppressed if we decided NOT to log tuple outputs.
…des. This code attempts to be a reasonable facsimile of the original commit. A subsequent commit will refactor so that unit tests can be added. Signed-off-by: Greg Schohn <[email protected]>
|
This has been superceded by #433 |
Description
This uses the metrics logging code (from #376 ) to add a new metric that contains the source and target response status codes and a "are they the same" value to make using this simpler.
It also fixes an issue discovered by sonarqube regarding outputting the entire stack trace in our metrics.
Issues Resolved
MIGRATIONS-1326
Testing
Manually tested
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.