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.

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

Jetty Connector Metrics are only reported onClosed #3873

Closed
kelunik opened this issue Jun 1, 2023 · 4 comments · Fixed by #4514
Closed

Jetty Connector Metrics are only reported onClosed #3873

kelunik opened this issue Jun 1, 2023 · 4 comments · Fixed by #4514
Labels
instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module

Comments

@kelunik
Copy link

kelunik commented Jun 1, 2023

Describe the bug

io.micrometer.core.instrument.binder.jetty.JettyConnectionMetrics reports metrics only in onClosed. Thus, connections receiving lots of traffic / messages but that are kept open, will never be visible in the metrics, then create a high peak at closure.

Environment

  • Micrometer version: 1.10.6
  • Micrometer registry: Simple + Elastic APM Agent
  • OS: Linux
  • Java version: 17.0.7

To Reproduce

Server server = new Server(createThreadPool());
ServerConnector connector = new ServerConnector(server, 2, -1, tls, alpn, h2);
connector.addBean(new JettyConnectionMetrics(MetricsRegistry.register(), connector, List.of()));
connector.setPort(1234);
server.addConnector(connector);

Keep the connection in the client open (e.g. OkHttp and send a request every minute - default idle time is 5 minutes).

Expected behavior

Traffic being visible in the reported metrics.

@marcingrzejszczak
Copy link
Contributor

So what would be the expected outcome? Would you want the metrics to be available as gauges?

@marcingrzejszczak marcingrzejszczak added the waiting for feedback We need additional information before we can continue label Dec 27, 2023
@kelunik
Copy link
Author

kelunik commented Dec 28, 2023

No, counters are fine, but metrics should be recorded when bytes are sent / received instead of on connection closure.

@marcingrzejszczak
Copy link
Contributor

I see, so instead of doing a listener on the connection, most likely we should be using the NetworkTrafficListener and use the incoming and outgoing methods?

public interface NetworkTrafficListener
{
    /**
     * <p>Callback method invoked when a connection from a remote client has been accepted.</p>
     * <p>The {@code socket} parameter can be used to extract socket address information of
     * the remote client.</p>
     *
     * @param socket the socket associated with the remote client
     */
    default void opened(Socket socket)
    {
    }

    /**
     * <p>Callback method invoked when bytes sent by a remote client arrived on the server.</p>
     *
     * @param socket the socket associated with the remote client
     * @param bytes the read-only buffer containing the incoming bytes
     */
    default void incoming(Socket socket, ByteBuffer bytes)
    {
    }

    /**
     * <p>Callback method invoked when bytes are sent to a remote client from the server.</p>
     * <p>This method is invoked after the bytes have been actually written to the remote client.</p>
     *
     * @param socket the socket associated with the remote client
     * @param bytes the read-only buffer containing the outgoing bytes
     */
    default void outgoing(Socket socket, ByteBuffer bytes)
    {
    }

marcingrzejszczak added a commit that referenced this issue Dec 29, 2023
before this change we were dumping in / out bytes from the connection upon the connection close which could result in big traffic whenever the connection got closed
after this change we're measuring the bytes through NetworkTrafficListener that hooks in whenever actual in / out bytes are being processed

fixes gh-3873
@marcingrzejszczak
Copy link
Contributor

marcingrzejszczak commented Dec 29, 2023

I'm no expert in Jetty, but after changing the JettyConnectionMetrics to also implement NetworkTrafficListener, and by changing its contract to call this

@Override
    public void incoming(Socket socket, ByteBuffer bytes) {
        bytesIn.record(bytes.limit());
    }

    @Override
    public void outgoing(Socket socket, ByteBuffer bytes) {
        bytesOut.record(bytes.limit());
    }

I can make the tests pass on the server side (I return the same amount of bytes), but I have a difference in bytes between what we are returning when the connection gets closed vs when the outgoing bytes get reported through the NetworkTrafficListener for the client side.

In case of JettyConnectionMetricsTest with metrics done on the connection close there it turns out that there are 2 connections being closed. One with 166 bytes getting dumped and the other with 618 bytes - together 784 bytes.

With the NTL changes we report only the 618 bytes (166 bytes are missing in comparison to the bytes before my change). Those are combined from 174 bytes and 442 bytes that are being gathered by 2 calls to the outgoing method.

Are there any Jetty experts that can explain this? Maybe I'm misusing the ByteBuffer. You can check the draft PR here

@marcingrzejszczak marcingrzejszczak added work-in-progress and removed waiting for feedback We need additional information before we can continue labels Dec 29, 2023
marcingrzejszczak added a commit that referenced this issue Dec 29, 2023
before this change we were dumping in / out bytes from the connection upon the connection close which could result in big traffic whenever the connection got closed
after this change we're measuring the bytes through NetworkTrafficListener that hooks in whenever actual in / out bytes are being processed

fixes gh-3873
marcingrzejszczak added a commit that referenced this issue Dec 29, 2023
before this change we were dumping in / out bytes from the connection upon the connection close which could result in big traffic whenever the connection got closed
after this change we're measuring the bytes through NetworkTrafficListener that hooks in whenever actual in / out bytes are being processed

fixes gh-3873
marcingrzejszczak added a commit that referenced this issue Dec 29, 2023
before this change we were dumping in / out bytes from the connection upon the connection close which could result in big traffic whenever the connection got closed
after this change we're measuring the bytes through NetworkTrafficListener that hooks in whenever actual in / out bytes are being processed

fixes gh-3873
marcingrzejszczak added a commit that referenced this issue Dec 29, 2023
before this change we were dumping in / out bytes from the connection upon the connection close which could result in big traffic whenever the connection got closed
after this change we're measuring the bytes through NetworkTrafficListener that hooks in whenever actual in / out bytes are being processed

fixes gh-3873
@shakuzen shakuzen added module: micrometer-core An issue that is related to our core module instrumentation An issue that is related to instrumenting a component and removed waiting-for-triage labels Mar 6, 2024
shakuzen pushed a commit that referenced this issue Apr 8, 2024
before this change we were dumping in / out bytes from the connection upon the connection close which could result in big traffic whenever the connection got closed
after this change we're measuring the bytes through NetworkTrafficListener that hooks in whenever actual in / out bytes are being processed.
This only works on the server-side connection for now. Future versions of Jetty may extend it to client side.

fixes gh-3873
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants