#6 Added Error Metrics #12
Conversation
|
@ezienecker can you review this change? |
|
@antrix190 before I review please correct the formatting |
|
@ezienecker Can you share the formatting standards followed? |
Done. |
|
@antrix190 Taking over from @ezienecker since he's not available this week. I added an I'll review the PR later today. |
Sure @pvorb I have formatted the files already. Please review and let me know if i have make changes |
| })).meterFilter(MeterFilter.deny(id -> { | ||
| String uri = id.getTag("uri"); | ||
| return uri != null && uri.contains("favicon"); | ||
| })); |
There was a problem hiding this comment.
Why do you filter out the /actuator and favicon URLs? This library doesn't have anything to do with spring-webmvc, so I guess this would be the task of the service that uses the library. Maybe the class can be deleted entirely?
There was a problem hiding this comment.
Okay i will remove the favicon and actuator url filtering but this Class defines a bean configuration to return MetricRegistryCustomizer. Not required to get the host region etc as a default.
There was a problem hiding this comment.
Where do the values for ${host}, ${service} and ${region} come from?
There was a problem hiding this comment.
Essentially from properties file.
| return "KafkaHealthProperties{" + "topic='" + topic + '\'' + ", sendReceiveTimeout=" + sendReceiveTimeout + | ||
| ", pollTimeout=" + pollTimeout + ", subscriptionTimeout=" + subscriptionTimeout + '}'; | ||
| return "KafkaHealthProperties{" + "topic='" + topic + '\'' + ", sendReceiveTimeout=" + sendReceiveTimeout | ||
| + ", pollTimeout=" + pollTimeout + ", subscriptionTimeout=" + subscriptionTimeout + '}'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Please revert the changes to this file as they're unnecessary.
There was a problem hiding this comment.
I moved this file to separate package. Health.config
Should this be reverted.
src/main/java/com/deviceinsight/kafka/health/KafkaConsumingHealthIndicator.java
Show resolved
Hide resolved
| public KafkaConsumingHealthIndicator(KafkaHealthProperties kafkaHealthProperties, | ||
| Map<String, Object> kafkaConsumerProperties, Map<String, Object> kafkaProducerProperties) { | ||
| Map<String, Object> kafkaConsumerProperties, Map<String, Object> kafkaProducerProperties, | ||
| @Autowired MeterRegistry meterRegistry) { |
There was a problem hiding this comment.
The @Autowired annotation is also unnecessary here. This class should be initialized explicitly.
There was a problem hiding this comment.
what i'm trying to do here is create a bean in MetricsConfig and autowiring it here through construtor injection. Let me know if you want me to change this approach.
There was a problem hiding this comment.
Yes, I understand. But the class should be instantiated as described in the README rather than by autowiring dependencies, since this might cause issues for some projects that have multiple MeterRegistry beans, for example.
src/main/java/com/deviceinsight/kafka/health/KafkaConsumingHealthIndicator.java
Show resolved
Hide resolved
|
@antrix190 any progress on this? |
|
Hey @ezienecker @pvorb I'll incorporate the suggested changes by this weekend. |
Added MeterRegistry to log Kafka exceptions.