Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support Jetty 12 in JettyConnectionMetrics #4324

Closed
shakuzen opened this issue Nov 9, 2023 · 0 comments · Fixed by #4325
Closed

Support Jetty 12 in JettyConnectionMetrics #4324

shakuzen opened this issue Nov 9, 2023 · 0 comments · Fixed by #4325
Assignees
@shakuzen
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Milestone
1.12.0

Comments

@shakuzen
Copy link
Member

shakuzen commented Nov 9, 2023

See #4261. Currently, if JettyConnectionMetrics is used with Jetty 12, a NoClassDefFoundError will happen whenever the onClosed callback of the Connection Listener is called.

INFO  o.e.j.i.AbstractConnection Failure while notifying listener imcibj.JettyConnectionMetrics@7786a3c6{STARTED} java.lang.NoClassDefFoundError: org/eclipse/jetty/server/HttpConnection

micrometer/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jetty/JettyConnectionMetrics.java

Lines 165 to 170 in 214f038

String serverOrClient = connection instanceof HttpConnection ? "server" : "client";
sample.stop(Timer.builder("jetty.connections.request")
.description("Jetty client or server requests")
.tag("type", serverOrClient)
.tags(tags)
.register(registry));

We are trying to determine whether the connection instrumented is a server or client connection, and we are doing that with an instanceof check against HttpConnection, which is in the jetty-server artifact. This is not very robust as there are other server implementations of Connection. It also causes the above mentioned error in conjunction with the relocation of the HttpConnection class to a different package.
@shakuzen shakuzen added enhancement A general enhancement module: micrometer-core An issue that is related to our core module labels Nov 9, 2023
@shakuzen shakuzen added this to the 1.12.0 milestone Nov 9, 2023
@shakuzen shakuzen mentioned this issue Nov 9, 2023
Closed
@shakuzen shakuzen self-assigned this Nov 9, 2023
shakuzen added a commit to shakuzen/micrometer that referenced this issue Nov 9, 2023
@shakuzen
Improve JettyConnectionMetrics connection type detection
60cd0d8 
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 added a commit to shakuzen/micrometer that referenced this issue Nov 9, 2023
@shakuzen
Improve JettyConnectionMetrics connection type detection
d5620fa 
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 added a commit to shakuzen/micrometer that referenced this issue Nov 9, 2023
@shakuzen
Improve JettyConnectionMetrics connection type detection
5d78074 
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 added a commit to shakuzen/micrometer that referenced this issue Nov 9, 2023
@shakuzen
Improve JettyConnectionMetrics connection type detection
41be35f 
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 added a commit to shakuzen/micrometer that referenced this issue Nov 9, 2023
@shakuzen
Improve JettyConnectionMetrics connection type detection
36ef458 
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 shakuzen mentioned this issue Nov 9, 2023
Merged
shakuzen added a commit that referenced this issue Nov 10, 2023
@shakuzen
Improve JettyConnectionMetrics connection type detection (#4325)
b284c4e 
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.

Adds a jetty12 module to the samples to run the `JettyConnectionMetricsTest` against Jetty 12.

Resolves gh-4324
@shakuzen shakuzen mentioned this issue Feb 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@shakuzen shakuzen

Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Projects
None yet
Milestone
1.12.0
Development

Successfully merging a pull request may close this issue.

Improve JettyConnectionMetrics connection type detection shakuzen/micrometer
1 participant
@shakuzen