WIP: Moved kamon-netty #1113
WIP: Moved kamon-netty #1113
Conversation
There was a problem hiding this comment.
There are many tiny details to figure out here and there, and three bigger questions to address:
- Is there a way this instrumentation can work with the latest Netty version? It will most certainly happen that some people will have newer versions in their classpath and as far as I understand, that would break this instrumentation. Some ideas that come to mind:
- Find a different way of extracting the event loop name for the metrics
- Add some conditions to ensure that event loop metrics will only be collected on supported versions and not fail on newer ones
- There are no tests for epoll-based loops. I saw the helper methods in the tests side but they are not being used. Is it because this doesn't support epoll or the tests should be duplicated for epoll and that should be it?
- What happens when someone is using a higher-level framework that builds on top of Netty (e.g. Armeria). Do we get double spans in that case? 🤔
| * - task-waiting-time: The waiting time in the queue. | ||
| */ | ||
| val registeredChannelsMetric = Kamon.rangeSampler("netty.event-loop.registered-channels") | ||
| val taskProcessingTimeMetric = Kamon.histogram("netty.event-loop.task-processing-time", time.nanoseconds) |
There was a problem hiding this comment.
Please rename this metric to netty.event-loop.task.duration. We will soon move to OTel-based conventions and for everything that measures a duration, the metric should end with duration.
| import kamon.instrumentation.http.{HttpClientInstrumentation, HttpOperationNameGenerator, HttpServerInstrumentation} | ||
| import kamon.util.DynamicAccess | ||
|
|
||
| object Netty { |
There was a problem hiding this comment.
Please remove all the stuff that is commented out
| @@ -0,0 +1,18 @@ | |||
| package kamon.netty.instrumentation.advisor; | |||
There was a problem hiding this comment.
Please move all the code to the kamon.instrumentation.netty package to follow the same conventions across all projects
|
|
||
| @OnMethodExit | ||
| static void onExit(@This Object eventLoop, @Return(readOnly = false) Queue<Runnable> queue) { | ||
| MonitoredQueue.apply((EventLoop) eventLoop, queue); |
There was a problem hiding this comment.
Is this the method that would fail on more recent Netty versions because newTaskQueue was turned into a static method?
| @@ -0,0 +1,17 @@ | |||
| <!DOCTYPE aspectj PUBLIC "-//AspectJ//DTD//EN" "http://www.eclipse.org/aspectj/dtd/aspectj.dtd"> | |||
| } | ||
| } | ||
|
|
||
| class DefaultNameGenerator extends HttpOperationNameGenerator { |
There was a problem hiding this comment.
Remove this class for the same reasons as mentioned above
| @@ -0,0 +1,8 @@ | |||
| package kamon.netty | |||
| onSubTypesOf("io.netty.handler.codec.http.HttpObjectEncoder") | ||
| .advise(method("encode").and(takesArguments(3)), classOf[ServerEncodeMethodAdvisor]) | ||
|
|
||
| // @After("execution(* io.netty.handler.codec.http.HttpObjectDecoder+.decode(..)) && args(ctx, *, out)") |
There was a problem hiding this comment.
Remove the commented code
| onType("io.netty.bootstrap.ServerBootstrap$ServerBootstrapAcceptor") | ||
| .advise(method("channelRead").and(takesArguments(2)), classOf[ServerChannelReadMethodAdvisor]) | ||
|
|
||
| // @Before("execution(* io.netty.bootstrap.ServerBootstrap.group(..)) && args(bossGroup, workerGroup)") |
| @@ -0,0 +1,26 @@ | |||
| package kamon.netty.instrumentation.mixin | |||
There was a problem hiding this comment.
This class should be removed and use HasContext from instrumentation-common instead
No description provided.