Migrate Metrics extension to a vert.x route #4174
Conversation
| Stream<String> acceptHeaders = request.headers().getAll("Accept").stream(); | ||
|
|
||
| try { | ||
| internalHandler.handleRequest(request.path(), metricsPath, request.rawMethod(), acceptHeaders, |
There was a problem hiding this comment.
Is the logic inside the internalHandler.handleRequest() non-blocking? If so then OK, otherwise you should use HandlerType#BLOCKING instead.
There was a problem hiding this comment.
Ok I added HandlerType#BLOCKING
There was a problem hiding this comment.
I think it's safer to use "blocking" for now. While it could be non-blocking, it the user declare a metric where the computation is blocking that could have a terrible impact.
| @@ -65,7 +69,7 @@ | |||
| static final class SmallRyeMetricsConfig { | |||
|
|
|||
| /** | |||
| * The path to the metrics Servlet. | |||
| * The path to metrics handler. | |||
| @Record(STATIC_INIT) | ||
| void createRoute(BuildProducer<RouteBuildItem> routes, | ||
| SmallRyeMetricsRecorder recorder) { | ||
| SmallRyeMetricsHandler metricsHandler = new SmallRyeMetricsHandler(); |
There was a problem hiding this comment.
Couldn't we push the path directly from config as a parameter?
The handler is then passed as a recorder parameter so we have to make sure it will work but a setter and a field would do it, I think.
There was a problem hiding this comment.
I now changed it so that the handler is instantiated in the recorder and a setter is used to set the path
|
There is a weird CI failure that I've been trying to track down but so far I've been unable to pinpoint it. Would it be possible to rebase this onto the latest Doing so fixed similar issues I had. I think there was some kind of weird caching of artifacts on CI |
jmartisk
force-pushed
the
master-issue-4159
branch
from
8dc9d58 to
76465c8
Compare
Rebased. Fingers crossed :) |
...etrics/runtime/src/main/java/io/quarkus/smallrye/metrics/runtime/SmallRyeMetricsHandler.java
Show resolved
Hide resolved
cescoffier
changed the title
|
Thanks @jmartisk ! |
Fixes #4159