Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.primitives.Ints;

import com.linecorp.armeria.client.metric.ClientRequestLifecycleListener;
import com.linecorp.armeria.client.proxy.ProxyConfig;
import com.linecorp.armeria.client.proxy.ProxyConfigSelector;
import com.linecorp.armeria.common.CommonPools;
Expand Down Expand Up @@ -134,6 +135,8 @@ public final class ClientFactoryBuilder implements TlsSetters {
private ClientTlsConfig tlsConfig;
private boolean staticTlsSettingsSet;
private boolean autoCloseConnectionPoolListener = true;
private ClientRequestLifecycleListener clientRequestLifecycleListener =
ClientRequestLifecycleListener.noop();

ClientFactoryBuilder() {
connectTimeoutMillis(Flags.defaultConnectTimeoutMillis());
Expand Down Expand Up @@ -260,6 +263,21 @@ public <T> ClientFactoryBuilder channelOption(ChannelOption<T> option, T value)
return this;
}

/**
* Sets the {@link ClientRequestLifecycleListener} that listens to the lifecycle events of
* client requests created by the {@link ClientFactory}.
*
* <p>If not set, {@link ClientRequestLifecycleListener#noop()} is used by default.
*
* @param listener the listener to be notified of client request lifecycle events. Must not be {@code null}.
* @return {@code this} to support method chaining.
*/
public ClientFactoryBuilder clientRequestLifecycleListener(ClientRequestLifecycleListener listener) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity.

Suggested change
public ClientFactoryBuilder clientRequestLifecycleListener(ClientRequestLifecycleListener listener) {
public ClientFactoryBuilder requestLifecycleListener(ClientRequestLifecycleListener listener) {

requireNonNull(listener, "listener");
clientRequestLifecycleListener = listener;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow multiple ClientRequestLifecycleListeners?

return this;
}
Comment on lines +267 to +280
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add @UnstableApi annotation to the new public method.

This new public API method should be annotated with @UnstableApi to signal that the API is still evolving. As per coding guidelines, newly added public methods require this annotation unless the class itself is already annotated or the method is in an internal/testing package.

🔎 Proposed fix
     /**
      * Sets the {@link ClientRequestLifecycleListener} that listens to the lifecycle events of
      * client requests created by the {@link ClientFactory}.
      *
      * <p>If not set, {@link ClientRequestLifecycleListener#noop()} is used by default.
      *
      * @param listener the listener to be notified of client request lifecycle events. Must not be {@code null}.
      * @return {@code this} to support method chaining.
      */
+    @UnstableApi
     public ClientFactoryBuilder clientRequestLifecycleListener(ClientRequestLifecycleListener listener) {
         requireNonNull(listener, "listener");
         clientRequestLifecycleListener = listener;
         return this;
     }

Based on coding guidelines, new public API methods require @UnstableApi annotation.

🤖 Prompt for AI Agents
In core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
around lines 267 to 280, the newly added public method
clientRequestLifecycleListener(...) is missing the @UnstableApi annotation
required for new public APIs; add the
@com.linecorp.armeria.common.annotation.UnstableApi annotation (or the correct
import) to the method declaration, ensure the import statement for UnstableApi
is present/updated, and keep the existing null check and return behavior
unchanged.


private void channelOptions(Map<ChannelOption<?>, Object> newChannelOptions) {
@SuppressWarnings("unchecked")
final ClientFactoryOptionValue<Map<ChannelOption<?>, Object>> castOptions =
Expand Down Expand Up @@ -1124,7 +1142,13 @@ private ClientFactoryOptions buildOptions() {
* Returns a newly-created {@link ClientFactory} based on the properties of this builder.
*/
public ClientFactory build() {
return new DefaultClientFactory(new HttpClientFactory(buildOptions(), autoCloseConnectionPoolListener));
return new DefaultClientFactory(
new HttpClientFactory(
buildOptions(),
autoCloseConnectionPoolListener,
clientRequestLifecycleListener
)
);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex
final DecodedHttpResponse res = new DecodedHttpResponse(eventLoop);
updateCancellationTask(ctx, req, res);

factory.clientRequestLifecycleListener().onRequestPending(ctx);
ctx.log().addListener(
factory.clientRequestLifecycleListener()
.asRequestLogListener()
);
Comment on lines +107 to +111
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Cache the lifecycle listener to avoid redundant method calls.

The listener is retrieved twice from the factory on the hot path. Store it in a local variable for efficiency.

🔎 Proposed fix
+        final ClientRequestLifecycleListener lifecycleListener = 
+                factory.clientRequestLifecycleListener();
-        factory.clientRequestLifecycleListener().onRequestPending(ctx);
+        lifecycleListener.onRequestPending(ctx);
         ctx.log().addListener(
-                factory.clientRequestLifecycleListener()
-                        .asRequestLogListener()
+                lifecycleListener.asRequestLogListener()
         );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
factory.clientRequestLifecycleListener().onRequestPending(ctx);
ctx.log().addListener(
factory.clientRequestLifecycleListener()
.asRequestLogListener()
);
final ClientRequestLifecycleListener lifecycleListener =
factory.clientRequestLifecycleListener();
lifecycleListener.onRequestPending(ctx);
ctx.log().addListener(
lifecycleListener.asRequestLogListener()
);
🤖 Prompt for AI Agents
In core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java around
lines 107 to 111, the call factory.clientRequestLifecycleListener() is invoked
twice on the hot path; cache it in a local variable (e.g.,
ClientRequestLifecycleListener listener =
factory.clientRequestLifecycleListener()) and then call
listener.onRequestPending(ctx) and
ctx.log().addListener(listener.asRequestLogListener()) to avoid redundant method
calls while preserving behavior.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Find ClientRequestLifecycleListener interface
fd -t f -e java -p . | xargs rg -l "interface ClientRequestLifecycleListener" 2>/dev/null | head -5

# Find HttpClientDelegate.java
fd -t f -name "HttpClientDelegate.java" 2>/dev/null

# Find any classes with "ClientRequestLifecycle" in their name
fd -t f -e java -p . | xargs rg -l "ClientRequestLifecycle" 2>/dev/null

Repository: line/armeria

Length of output: 741


🏁 Script executed:

# Read ClientRequestLifecycleListener.java
echo "=== ClientRequestLifecycleListener.java ==="
head -30 core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestLifecycleListener.java

echo -e "\n=== DefaultClientRequestLogListener.java ==="
head -30 core/src/main/java/com/linecorp/armeria/client/logging/DefaultClientRequestLogListener.java

echo -e "\n=== ClientRequestMetrics.java ==="
head -30 core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java

echo -e "\n=== NoopClientRequestLifecycleListener.java ==="
head -30 core/src/main/java/com/linecorp/armeria/client/metric/NoopClientRequestLifecycleListener.java

# Check HttpClientDelegate.java location and lines around 107-111
echo -e "\n=== HttpClientDelegate.java (around lines 100-115) ==="
sed -n '100,115p' core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java

Repository: line/armeria

Length of output: 5927


🏁 Script executed:

# Get the full ClientRequestLifecycleListener interface declaration
echo "=== ClientRequestLifecycleListener interface ==="
sed -n '1,50p' core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestLifecycleListener.java | tail -25

# Get the full DefaultClientRequestLogListener class declaration
echo -e "\n=== DefaultClientRequestLogListener class declaration ==="
sed -n '1,65p' core/src/main/java/com/linecorp/armeria/client/logging/DefaultClientRequestLogListener.java | tail -25

# Get the asRequestLogListener method signature
echo -e "\n=== asRequestLogListener method in ClientRequestLifecycleListener ==="
rg -A5 "asRequestLogListener" core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestLifecycleListener.java

# Check ClientRequestMetrics for @UnstableApi
echo -e "\n=== ClientRequestMetrics @UnstableApi check ==="
rg "@UnstableApi|^public class ClientRequestMetrics" core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java | head -5

Repository: line/armeria

Length of output: 3016


🏁 Script executed:

# Check ClientRequestMetrics full class declaration with annotations
echo "=== ClientRequestMetrics full header ==="
sed -n '1,45p' core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java

# Also check if there are any other public classes/methods in ClientRequestLifecycleListener
echo -e "\n=== Full ClientRequestLifecycleListener interface ==="
wc -l core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestLifecycleListener.java

# Get full interface
cat core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestLifecycleListener.java | head -150

Repository: line/armeria

Length of output: 6333


🏁 Script executed:

# Check if ClientRequestMetrics has @UnstableApi annotation
echo "=== ClientRequestMetrics class with preceding annotations ==="
rg -B2 "^public class ClientRequestMetrics" core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java

# Also verify DefaultClientRequestLogListener has @UnstableApi
echo -e "\n=== DefaultClientRequestLogListener with annotations ==="
rg -B2 "^public class DefaultClientRequestLogListener" core/src/main/java/com/linecorp/armeria/client/logging/DefaultClientRequestLogListener.java

# Check if DefaultClientRequestLogListener is in logging package (not .internal)
echo -e "\n=== Package declarations ==="
rg "^package" core/src/main/java/com/linecorp/armeria/client/logging/DefaultClientRequestLogListener.java
rg "^package" core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java
rg "^package" core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestLifecycleListener.java

Repository: line/armeria

Length of output: 523


🏁 Script executed:

# Get the full ClientRequestMetrics class definition to confirm it's missing @UnstableApi
echo "=== ClientRequestMetrics lines 30-50 ==="
sed -n '30,50p' core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java

# Double-check by looking at all annotations before the class
echo -e "\n=== ClientRequestMetrics all lines before class declaration ==="
grep -n "@\|^public class" core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java | head -10

Repository: line/armeria

Length of output: 1488


Add @UnstableApi annotation to ClientRequestMetrics.

The ClientRequestMetrics class at core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java is missing the @UnstableApi annotation. Per coding guidelines, all newly added public classes must be annotated with @UnstableApi unless they are located in .internal or .testing packages or test source sets. The related classes ClientRequestLifecycleListener, DefaultClientRequestLogListener, and NoopClientRequestLifecycleListener are correctly annotated.

🤖 Prompt for AI Agents
In
core/src/main/java/com/linecorp/armeria/client/metric/ClientRequestMetrics.java
around lines 1 to 999 (the class declaration), the public class
ClientRequestMetrics is missing the required @UnstableApi annotation; add the
annotation above the class declaration, import
com.linecorp.armeria.common.annotation.UnstableApi (or the correct package used
in the project), and run a quick compile to ensure there are no import or symbol
issues. Ensure you do not annotate classes in .internal or .testing packages,
but since this is a public non-internal class, the annotation must be applied.


try {
resolveProxyConfig(protocol, endpoint, ctx, (proxyConfig, thrown) -> {
if (thrown != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.google.common.collect.MapMaker;

import com.linecorp.armeria.client.endpoint.EndpointGroup;
import com.linecorp.armeria.client.metric.ClientRequestLifecycleListener;
import com.linecorp.armeria.client.proxy.ProxyConfigSelector;
import com.linecorp.armeria.client.redirect.RedirectConfig;
import com.linecorp.armeria.common.Http1HeaderNaming;
Expand Down Expand Up @@ -142,14 +143,20 @@ private static void setupTlsMetrics(List<X509Certificate> certificates, MeterReg
private final ClientFactoryOptions options;
private final boolean autoCloseConnectionPoolListener;
private final AsyncCloseableSupport closeable = AsyncCloseableSupport.of(this::closeAsync);
private final ClientRequestLifecycleListener clientRequestLifecycleListener;

HttpClientFactory(ClientFactoryOptions options, boolean autoCloseConnectionPoolListener) {
this(options, autoCloseConnectionPoolListener, ClientRequestLifecycleListener.noop());
}

HttpClientFactory(ClientFactoryOptions options, boolean autoCloseConnectionPoolListener,
ClientRequestLifecycleListener clientRequestLifecycleListener) {
workerGroup = options.workerGroup();

@SuppressWarnings("unchecked")
final AddressResolverGroup<InetSocketAddress> group =
(AddressResolverGroup<InetSocketAddress>) options.addressResolverGroupFactory()
.apply(workerGroup);
.apply(workerGroup);
addressResolverGroup = group;

final Bootstrap bootstrap = new Bootstrap();
Expand Down Expand Up @@ -189,10 +196,10 @@ private static void setupTlsMetrics(List<X509Certificate> certificates, MeterReg
final TlsEngineType tlsEngineType = options.tlsEngineType();
sslCtxHttp1Or2 = SslContextUtil
.createSslContext(SslContextBuilder::forClient, false, tlsEngineType,
tlsAllowUnsafeCiphers, tlsCustomizer, keyCertChainCaptor);
tlsAllowUnsafeCiphers, tlsCustomizer, keyCertChainCaptor);
sslCtxHttp1Only = SslContextUtil
.createSslContext(SslContextBuilder::forClient, true, tlsEngineType,
tlsAllowUnsafeCiphers, tlsCustomizer, keyCertChainCaptor);
tlsAllowUnsafeCiphers, tlsCustomizer, keyCertChainCaptor);
setupTlsMetrics(keyCertChainCaptor, options.meterRegistry());

final TlsProvider tlsProvider = options.tlsProvider();
Expand All @@ -202,7 +209,7 @@ private static void setupTlsMetrics(List<X509Certificate> certificates, MeterReg
clientTlsConfig = null;
}
sslContextFactory = new SslContextFactory(tlsProvider, options.tlsEngineType(), clientTlsConfig,
options.meterRegistry());
options.meterRegistry());
} else {
sslContextFactory = null;
}
Expand Down Expand Up @@ -232,6 +239,7 @@ private static void setupTlsMetrics(List<X509Certificate> certificates, MeterReg
this.autoCloseConnectionPoolListener = autoCloseConnectionPoolListener;

this.options = options;
this.clientRequestLifecycleListener = clientRequestLifecycleListener;

clientDelegate = new HttpClientDelegate(this, addressResolverGroup);
RequestTargetCache.registerClientMetrics(meterRegistry);
Expand Down Expand Up @@ -339,6 +347,10 @@ Http1HeaderNaming http1HeaderNaming() {
return http1HeaderNaming;
}

ClientRequestLifecycleListener clientRequestLifecycleListener() {
return clientRequestLifecycleListener;
}

@VisibleForTesting
AddressResolverGroup<InetSocketAddress> addressResolverGroup() {
return addressResolverGroup;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
/*
* Copyright 2025 LY Corporation
*
* LY Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.client.metric;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.common.ResponseHeaders;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.annotation.UnstableApi;
import com.linecorp.armeria.common.logging.RequestLogListener;

/**
* Listens to the lifecycle events of client requests.
*
* <p>This interface provides a high-level view of a request's lifecycle (e.g., pending, started, completed).
* unlike {@link RequestLogListener} which provides low-level property changes.
*
* <p>Note: The methods in this interface are typically not invoked directly by the
* transport layer. Instead, they are triggered by inspecting the changes in
* {@link com.linecorp.armeria.common.logging.RequestLog}.
* Implementations should use {@link #asRequestLogListener()} to bridge
* {@link com.linecorp.armeria.common.logging.RequestLogProperty}
* changes to these lifecycle methods.
*
* <p>For example, a standard implementation might map:
* <ul>
* <li>{@link com.linecorp.armeria.common.logging.RequestLogProperty#REQUEST_FIRST_BYTES_TRANSFERRED_TIME}
* to {@link #onRequestStart(ClientRequestContext)}</li>
* <li>{@link com.linecorp.armeria.common.logging.RequestLogProperty#ALL_COMPLETE}
* to {@link #onRequestComplete(ClientRequestContext, Throwable)}</li>
* </ul>
*
* @see com.linecorp.armeria.client.ClientFactoryBuilder#clientRequestLifecycleListener(
* ClientRequestLifecycleListener)
*/
@UnstableApi
public interface ClientRequestLifecycleListener {

/**
* Invoked when a request is created and scheduled for execution but has not yet started.
* Note: This method is explicitly invoked by HttpClientDelegate when
* HttpClientDelegate starts to call execute().
*/
void onRequestPending(ClientRequestContext ctx);

/**
* Called when the client begins execution (connection acquired, headers sent).
*/
void onRequestStart(ClientRequestContext ctx);

/**
* Called when the request is fully sent.
*/
void onRequestSendComplete(ClientRequestContext ctx);

/**
* Called when the first response headers are received.
*/
void onResponseHeaders(ClientRequestContext ctx, ResponseHeaders headers);

/**
* Called when the full response body is received successfully.
*/
void onResponseComplete(ClientRequestContext ctx);

/**
* Called when a request is completed with either success or failure.
*/
void onRequestComplete(ClientRequestContext ctx, @Nullable Throwable cause);

/**
* Returns a {@link RequestLogListener} that delegates
* {@link com.linecorp.armeria.common.logging.RequestLog}
* events to this lifecycle listener.
* This method bridges the low-level {@link com.linecorp.armeria.common.logging.RequestLog}
* updates to the high-level lifecycle methods
* defined in this interface. The returned listener is registered to the {@link ClientRequestContext}
* automatically when the request starts.
*/
RequestLogListener asRequestLogListener();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove this from public API but convert this to RequestLogListener internally? Users are only interested in implementing the callbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone might want to define and use their own RequestLogListener.
For now, I revised it in a way that provides a default method so users don’t have to worry about it as much.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need such a extension. What do you think of considering it later if necessary?


/**
* Returns a {@link ClientRequestMetrics} that collects the number of pending and active requests.
*/
static ClientRequestMetrics counting() {
return new ClientRequestMetrics();
}

/**
* Returns a {@link ClientRequestLifecycleListener} that does nothing.
*/
static ClientRequestLifecycleListener noop() {
return NoopClientRequestLifecycleListener.INSTANCE;
}

/**
* A {@link ClientRequestLifecycleListener} that does nothing.
*/
class NoopClientRequestLifecycleListener implements ClientRequestLifecycleListener {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this internal implementation to the top level class? NoopClientRequestLifecycleListener will be exposed to users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed it!


private static final NoopClientRequestLifecycleListener INSTANCE =
new NoopClientRequestLifecycleListener();
private final RequestLogListener requestLogListener = ((property, log) -> {});

@Override
public void onRequestPending(ClientRequestContext ctx) {
// no-op
}

@Override
public void onRequestStart(ClientRequestContext ctx) {
// no-op
}

@Override
public void onRequestSendComplete(ClientRequestContext ctx) {
// no-op
}

@Override
public void onResponseHeaders(ClientRequestContext ctx, ResponseHeaders headers) {
// no-op
}

@Override
public void onResponseComplete(ClientRequestContext ctx) {
// no-op
}

@Override
public void onRequestComplete(ClientRequestContext ctx, @Nullable Throwable cause) {
// no-op
}

@Override
public RequestLogListener asRequestLogListener() {
// no-op
return requestLogListener;
}
}
}
Loading
Loading