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 12 is missing a way to record server latencies #8069

Closed
lorban opened this issue May 30, 2022 · 6 comments
Closed

Jetty 12 is missing a way to record server latencies #8069

lorban opened this issue May 30, 2022 · 6 comments
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@lorban
Copy link
Contributor

lorban commented May 30, 2022

Jetty version(s)
12.0.x

Description
12.0.x currently does not have anything equivalent to 11's HttpChannel.Listener API which is needed at least to record server latencies with our performance tests.

@lorban lorban added Bug For general bugs on Jetty side jetty 12 labels May 30, 2022
@lorban lorban self-assigned this May 30, 2022
@lorban
Copy link
Contributor Author

lorban commented May 30, 2022

This actually can be done with the existing wrapper mechanisms by creating a Handler.Wrapper that adds a stream wrapper:

    @Override
    public Request.Processor handle(Request request) throws Exception
    {
        long before = System.nanoTime();
        Request.Processor processor = super.handle(request);
        if (processor == null)
            return null;

        request.addHttpStreamWrapper(httpStream -> new HttpStream.Wrapper(httpStream)
        {
            @Override
            public void succeeded()
            {
                super.succeeded();
                recordLatency(System.nanoTime() - before);
            }

            @Override
            public void failed(Throwable x)
            {
                super.failed(x);
                recordLatency(System.nanoTime() - before);
            }
        });

        return processor;
    }

@lorban lorban closed this as completed May 30, 2022
@sbordet
Copy link
Contributor

sbordet commented May 30, 2022

@lorban can you make this a utility Handler with overridable methods such as onRequestBegin(), etc. so people that wants to use it can do so overriding only the few methods they need?

@sbordet sbordet reopened this May 30, 2022
@gregw
Copy link
Contributor

gregw commented May 31, 2022

It could have a bunch of overridable methods that correspond to the previous listener events... or is that just adding complexity.
Perhaps just have an extensible method that creates the actual listener would be sufficient?

@sbordet
Copy link
Contributor

sbordet commented Jun 3, 2022

@gregw won't you still need a Listener interface with all the onXYZ() methods?

@gregw
Copy link
Contributor

gregw commented Jun 3, 2022

It could be done with a new listener interface, or just an extendible method to create the stream listener.

My preference is to avoid lots of lists of listeners that we must iterate over.

@olamy olamy moved this to In progress in Jetty 12.0.ALPHAS Sep 7, 2022
lorban added a commit that referenced this issue Jan 16, 2023
lorban added a commit that referenced this issue Jan 18, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jan 18, 2023
lorban added a commit that referenced this issue Jan 18, 2023
lorban added a commit that referenced this issue Jan 18, 2023
lorban added a commit that referenced this issue Jan 18, 2023
lorban added a commit that referenced this issue Jan 18, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jan 18, 2023
lorban added a commit that referenced this issue Jan 18, 2023
lorban added a commit that referenced this issue Jan 19, 2023
* #8069 add latency recording handler

Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban moved this to ✅ Done in Jetty 12.0.0.beta0 - FROZEN Jan 26, 2023
@lorban lorban closed this as completed Jan 26, 2023
@lorban
Copy link
Contributor Author

lorban commented Jan 26, 2023

Introduced AbstractLatencyRecordingHandler in #9172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

No branches or pull requests

3 participants