Improve JettyConnectionMetrics connection type detection
#4325
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `HttpConnection` class was relocated to a different package in Jetty 12 which was causing a `NoClassDefFoundError` when doing an instanceof check on it. The goal of it is to determine whether the connection instrumented is a server connection or a client connection. However, it specifically checks against `HttpConnection` and assumes that all server connections will be an instanceof `HttpConnection` and any not an instanceof it are client connections. This is brittle because Jetty supports more implementations of `Connection` on the server side than `HttpConnection` and there could in theory be an arbitrary implementation where we do not know whether it is a server or client connection (it could also be neither). This instead checks whether the package name contains `server` or `client` or neither. This is admittedly also brittle, but given the known implementations of `Connection` provided by the Jetty project, this pattern generally seems to hold, and it is at least more honest using `UNKNOWN` when our heuristic fails. It also avoids the `NoClassDefFoundError` when using `JettyConnectionMetrics` with Jetty 12. Resolves micrometer-metricsgh-4324
shakuzen
commented
Nov 9, 2023
| @@ -2,11 +2,7 @@ description 'Micrometer instrumentation for Jetty 11' | |||
|
|
|||
| dependencies { | |||
| api project(":micrometer-core") | |||
| api(libs.jetty11Server) { | |||
| version { | |||
| strictly libs.jetty11Server.get().version | |||
There was a problem hiding this comment.
This was no longer needed since I removed the jetty dependencies from dependencies.gradle in lieu of referring to the type-safe version catalog everywhere for jetty.
shakuzen
commented
Nov 9, 2023
| @@ -162,10 +161,16 @@ public void onClosed(Connection connection) { | |||
| } | |||
|
|
|||
| if (sample != null) { | |||
| String serverOrClient = connection instanceof HttpConnection ? "server" : "client"; | |||
| String type = "UNKNOWN"; | |||
| if (connection.getClass().getName().contains("server")) { | |||
There was a problem hiding this comment.
We may want to consider switching to make the qualified type name the value of the type tag, but that would be breaking for anyone that relies on the current values. Hence I thought it better to consider that in a future version. This change should keep the prior values in most common cases, I hope.
|
The jdk11 build is failing because Jetty 12 requires Java 17. We could skip the jetty12 sample on jdk <17 or we could remove the jdk11 build. |
jonatan-ivanov
approved these changes
Nov 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
HttpConnectionclass was relocated to a different package in Jetty 12 which was causing aNoClassDefFoundErrorwhen doing an instanceof check on it. The goal of it is to determine whether the connection instrumented is a server connection or a client connection. However, it specifically checks againstHttpConnectionand assumes that all server connections will be an instanceofHttpConnectionand any not an instanceof it are client connections. This is brittle because Jetty supports more implementations ofConnectionon the server side thanHttpConnectionand there could in theory be an arbitrary implementation where we do not know whether it is a server or client connection (it could also be neither).This instead checks whether the package name contains
serverorclientor neither. This is admittedly also brittle, but given the known implementations ofConnectionprovided by the Jetty project, this pattern generally seems to hold, and it is at least more honest usingUNKNOWNwhen our heuristic fails. It also avoids theNoClassDefFoundErrorwhen usingJettyConnectionMetricswith Jetty 12.Resolves gh-4324