Aerospike Module Storage #184
Conversation
AntoxaAntoxic
force-pushed
the
aerospike-module-storage
branch
from
a9127ea to
a01b2e3
Compare
| meterForTag(prefix, measurementTag).increment(); | ||
| } | ||
|
|
||
| public MetricsRecorderTimer createRequestTimerForTag(final String prefix) { |
There was a problem hiding this comment.
choose one of these two createRequestTimerForTag and createRequestTimerForServiceType and use it everywhere it's needed
| TEXT("pbc.${prefix}.text"), | ||
| XML("pbc.${prefix}.xml"), | ||
| ERROR_SECONDARY_WRITE("pbc.err.secondaryWrite"), | ||
| ERROR_EXISTING_ID("pbc.err.existingId"), |
There was a problem hiding this comment.
mark ERROR_EXISTING_ID as deprecated
| return ServerResponse.status(HttpStatus.UNAUTHORIZED).build(); | ||
| } | ||
|
|
||
| metricsRecorder.recordMetric(MeasurementTag.REQUEST); |
There was a problem hiding this comment.
it's better to log this metric right before the moduleRepository.save call because it might be the case when the body is empty and it should be MeasurementTag.ERROR_BAD_REQUEST instead
There was a problem hiding this comment.
@AntoxaAntoxic It depends on whether we want to count all requests or only valid ones
| } | ||
|
|
||
| metricsRecorder.recordMetric(MeasurementTag.REQUEST); | ||
| final var timerContext = metricsRecorder.createRequestTimer(); |
There was a problem hiding this comment.
replace var with class name
| if (wrapper.getPayload().getType().equals(PayloadType.JSON.toString())) { | ||
| metricsRecorder.recordMetric(MeasurementTag.JSON); | ||
| return ServerResponse.ok().body(fromValue(wrapper.getPayload())); | ||
| } else if (wrapper.getPayload().getType().equals(PayloadType.XML.toString())) { | ||
| metricsRecorder.recordMetric(MeasurementTag.XML); | ||
| return ServerResponse.ok().body(fromValue(wrapper.getPayload())); | ||
| } else if (wrapper.getPayload().getType().equals(PayloadType.TEXT.toString())) { | ||
| metricsRecorder.recordMetric(MeasurementTag.TEXT); | ||
| return ServerResponse.ok().body(fromValue(wrapper.getPayload())); | ||
| } | ||
|
|
||
| return Mono.error(new UnsupportedMediaTypeException("Unsupported Media Type.")); |
| return ServerResponse.status(HttpStatus.UNAUTHORIZED).build(); | ||
| } | ||
|
|
||
| metricsRecorder.recordMetric(MeasurementTag.REQUEST); |
There was a problem hiding this comment.
@AntoxaAntoxic It depends on whether we want to count all requests or only valid ones
| if (error instanceof DuplicateKeyException) { | ||
| recordMetric(MeasurementTag.ERROR_DUPLICATE_KEY); | ||
| } else if (error instanceof RepositoryException) { | ||
| recordMetric(MeasurementTag.ERROR_DB); | ||
| } else if (error instanceof ResourceNotFoundException || error instanceof BadRequestException) { | ||
| recordMetric(MeasurementTag.ERROR_BAD_REQUEST); | ||
| } else if (error instanceof TimeoutException) { | ||
| recordMetric(MeasurementTag.ERROR_TIMED_OUT); | ||
| } else { | ||
| recordMetric(MeasurementTag.ERROR_UNKNOWN); | ||
| } |
There was a problem hiding this comment.
switch operator, if we are using java 21+
| private final MetricsRecorder metricsRecorder; | ||
| private final String prefix; | ||
|
|
||
| public Mono<ServerResponse> handleErrorMetrics(final Throwable error, final ServerRequest request) { |
There was a problem hiding this comment.
remove final modifiers from method params, also check for other occurrences
| } else if (response.statusCode() == HttpStatus.INTERNAL_SERVER_ERROR) { | ||
| recordMetric(MeasurementTag.ERROR_UNKNOWN); | ||
| } else if (response.statusCode() == HttpStatus.BAD_REQUEST) { | ||
| recordMetric(MeasurementTag.ERROR_BAD_REQUEST); | ||
| } else if (response.statusCode() == HttpStatus.NOT_FOUND) { | ||
| recordMetric(MeasurementTag.ERROR_MISSING_ID); | ||
| } |
| return responseBuilder.error(Mono.just(error), request) | ||
| .doOnNext(response -> handleErrorStatusCodes(request, response)); |
There was a problem hiding this comment.
I don't like an idea to "hide" response construction in some cases inside StorageMetricsRecorder
No description provided.