Conversation
fixed tests
| @ImportResource("classpath:spring-repository-bean.xml") | ||
| @EnableAutoConfiguration | ||
| @ComponentScan | ||
| @Slf4j |
There was a problem hiding this comment.
This annotation looks redundant as well, isn't it?
| ServiceType type; | ||
| static String ID_KEY = "uuid"; | ||
|
|
||
| protected String METRIC_TAG_PREFIX; |
There was a problem hiding this comment.
This field is not a static constant so the name should follow usual camel-case naming pattern.
| log.error(t.getMessage(), t); | ||
| // skip overwrite, report first prebid error | ||
| if (t instanceof PrebidException) { | ||
| applyMetrics(t); |
There was a problem hiding this comment.
Perhaps it's better to move this logic to handleErrorMetrics ?
There was a problem hiding this comment.
will do this later, it'll take some time to re-factor
| @Component | ||
| public class GraphiteMetricsRecorder extends MetricsRecorder | ||
| { | ||
| protected static final String PREFIX_PLACEHOLDER = "prefix"; |
There was a problem hiding this comment.
For expressiveness I would recommend making prefix more discernible, for example ${prefix} or alike.
| JSON_REQUEST_RATE("json-request_rate"), | ||
| XML_REQUEST_RATE("xml-request_rate"); | ||
| REQUEST_DURATION("pbc.prefix.request.duration"), | ||
| REQUEST_RATE("pbc.prefix.request"), |
There was a problem hiding this comment.
Now that we got rid of _rate suffix in metric name it's better to remove _RATE suffix from enum names as well to avoid confusion. This applies to names following too.
| REQUEST_INVALID_RATE("request.invalid"), | ||
| JSON_RATE("pbc.prefix.json"), | ||
| XML_RATE("pbc.prefix.xml"), | ||
| SYSTEM_ERR_RATE("pbc.prefix.err.db"); |
There was a problem hiding this comment.
To be consistent this one should become ERROR_DB.
| XML_RATE("pbc.prefix.xml"), | ||
| SYSTEM_ERR_RATE("pbc.prefix.err.db"); | ||
|
|
||
| @Getter @Setter private String tag; |
There was a problem hiding this comment.
Is @Setter necessary here?
| } | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
I think this is redundant since lombok's @Value takes care of generating these methods. Is there a good reason to replace them with own implementation?
There was a problem hiding this comment.
there is an issue with tests because of logic in value getter
|
|
||
| private <T>Mono<T> handleAerospikeError(Throwable throwable) { | ||
| if (throwable instanceof AerospikeException) { | ||
| return Mono.error(new RepositoryException(throwable.toString())); |
There was a problem hiding this comment.
It is a good idea to pass not only exception message to RepositoryException but original exception as well so that it is not lost if someone wants to log it later.
| return createReactive().setex(normalizedId, expiry, Json.toJson(wrapper)) | ||
| .map(payload -> wrapper); | ||
| } catch (RedisException e) { | ||
| return Mono.error(new RepositoryException(e.toString())); |
There was a problem hiding this comment.
It is a good idea to pass not only exception message to RepositoryException but original exception as well so that it is not lost if someone wants to log it later. This applies to similar block in findById
2 participants
added and changed metrics
fixed tests