Netty 3.8-3.10 instrumentation#1327
Conversation
devinsba
changed the title
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher<ClassLoader> classLoaderMatcher() { |
There was a problem hiding this comment.
Not necessarily for this PR, but now, that having a classLoaderMatcher check is the norm to combat matching costs. I think we should make the concept of key class for a library a required part of almost every instrumentation.
I also think we should revisit the idea of a Library as a first class concept as well. While at first, I think this seems like more work. I think right now we end up duplicating some classLoaderMatcher code because we lack the concept of a library.
| try (final AgentScope scope = activateSpan(span, false)) { | ||
| DECORATE.onResponse(span, (HttpResponse) msg.getMessage()); | ||
| DECORATE.beforeFinish(span); | ||
| span.finish(); |
There was a problem hiding this comment.
I think the span needs to be finished while it's in scope. Not 100% sure on that with our impl but it's a common pattern I've seen
There was a problem hiding this comment.
This code is the same in the other 2 versions of the netty instrumentation
|
|
||
| @Override | ||
| protected URI url(final HttpRequest request) throws URISyntaxException { | ||
| final URI uri = new URI(request.getUri()); |
There was a problem hiding this comment.
Probably not for this PR, but we probably shouldn't build URI just to extract pieces to tags later.
That creates a hot allocation point under load.
| } | ||
|
|
||
| @Override | ||
| protected Integer status(final HttpResponse httpResponse) { |
There was a problem hiding this comment.
Again, probably not this PR.
But why does this return Integer rather than int?
Also, we might want to create our Integer cache for HttpStatus codes, since most of them lie outside the default cache range.
|
I'm having a bit of trouble following the span.finish / scope.close logic. |
…ck (+forward port)
|
One of the reasons that span close or scope close might happen inside the try and not in finally is that they may need to be done while a specific scope (the one from the try-with-resources) is on top of the stack, so then we need to do the close within the try because the resource is closed before the finally is executed |
Yes, I was just suspicious because I know we've had some issues with memory leaks related to unclosed scopes. I don't doubt that this is normal pattern in our tracer, but I'm wondering if those are slow leaks from exception handling. Anyway, we can revisit that matter in another PR if necessary. |
5 participants
Creates new Netty 3.8 through 3.10 instrumentation, allowing for instrumentation of Play 2.3 which depends on this. Mostly copies the netty 4.0 instrumentation