[WIP]: Add OpenTelemetry metrics support for XDS Client #1127

aadityaraj7769 · 2025-11-23T05:49:11Z

Summary

This PR adds OpenTelemetry (OTel) metrics instrumentation to the XDS Client.

Changes

New Interface: Added XdsClientOtelMetricsProvider interface for collecting XDS client metrics via OpenTelemetry
- Connection lifecycle events (connection lost, closed, reconnection)
- Request/response tracking
- Error tracking (resource not found, invalid resource)
- Server latency measurements
- Connection state monitoring
- Initial wait time tracking for resource fetching
No-op Implementation: Added NoOpXdsClientOtelMetricsProvider as default implementation when metrics are disabled
Integration:
- Integrated metrics provider into XdsClientImpl constructor with dependency injection pattern
- Added metrics recording throughout XDS client lifecycle (connection events, resource updates, latency tracking)
- Metrics provider passed to XdsClientJmx for JMX integration
- Added builder support in D2ClientBuilder via setXdsClientOtelMetricsProvider()
Metrics Tracked:
- Counters: connection events, requests/responses, errors (not found/invalid resources), IRV sent count
- Histogram: server latency
- Gauges: connection state, active initial wait time

Backward Compatibility

Fully backward compatible - defaults to no-op provider when not configured

About XdsSensor

The XdsClient Sensor tracks metrics for xDS-based service discovery and client-side operations. It monitors connection events, request/response counts, and latency distributions for xDS service calls. This sensor enables client-side observability for distributed service communications using xDS.

New Otel Metrics

Counters

Xds.Client.ErrorCount
Xds.Client.ConnectionStateChangedCount
Xds.Client.RequestCount
Xds.Client.ResponseReceivedCount
Xds.Client.InitialResourceVersionSentCount

Histogram

Xds.Server.Latency

Gauges

Xds.Client.DisconnectedFromXdsServer
Xds.Client.ActiveInitialWaitTimeMs

…dling in XdsClientImpl

Kevin-Ngo · 2025-12-03T17:32:13Z

Can we please add some context into the PR description around

What the Xds metrics are used for?
What are the new metrics now?
How was this tested?

Kevin-Ngo · 2025-12-03T18:01:25Z

d2/src/main/java/com/linkedin/d2/balancer/D2ClientConfig.java


  public boolean subscribeToUriGlobCollection = false;
  public XdsServerMetricsProvider _xdsServerMetricsProvider = new NoOpXdsServerMetricsProvider();
+  public XdsClientOtelMetricsProvider _xdsClientOtelMetricsProvider = new NoOpXdsClientOtelMetricsProvider();


For public variables, we shouldn't prefix with _, that's the convention for private ones

I have followed the existing logic same as _xdsServerMetricsProvider which is basically for recording the xdsServer latency.

Yes, but this is new code and should follow better standards.. that last one was likely incorrectly committed.

Kevin-Ngo · 2025-12-03T18:11:35Z

d2/src/main/java/com/linkedin/d2/balancer/D2ClientConfig.java


  public boolean subscribeToUriGlobCollection = false;
  public XdsServerMetricsProvider _xdsServerMetricsProvider = new NoOpXdsServerMetricsProvider();
+  public XdsClientOtelMetricsProvider _xdsClientOtelMetricsProvider = new NoOpXdsClientOtelMetricsProvider();


Can we also add docstring above this variable to explain how it is used here?

Kevin-Ngo · 2025-12-03T18:11:54Z

d2/src/main/java/com/linkedin/d2/jmx/D2ClientJmxManager.java

+    // Get the client name from global prefix
+    String clientName = getGlobalPrefix(null);
+    xdsClientJmx.setClientName(clientName);
+


Why is it needed to set here? Do we need to set it anywhere else?

I set the client name because I want to use it in the XdsClientImpl file to record the server latency for this specific client. The clientName is one of the dimensions used in Otel for this metric.

Kevin-Ngo · 2025-12-03T18:13:03Z

d2/src/main/java/com/linkedin/d2/jmx/NoOpXdsClientOtelMetricsProvider.java

+ */
+public class NoOpXdsClientOtelMetricsProvider implements XdsClientOtelMetricsProvider {
+
+  @Override


add docstring {@inheritDoc} for all of these methods

Kevin-Ngo · 2025-12-03T18:15:03Z

d2/src/main/java/com/linkedin/d2/jmx/XdsClientJmx.java

+  public void setClientName(String clientName) {
+    _clientName = clientName;
+  }
+
+  public String getClientName() {
+    return _clientName;
  }


missing docstring

Kevin-Ngo · 2025-12-03T18:19:07Z

d2/src/main/java/com/linkedin/d2/xds/XdsClientImpl.java

    }

-    _xdsClientJmx = new XdsClientJmx(serverMetricsProvider);
+    _xdsClientJmx = new XdsClientJmx(serverMetricsProvider, xdsClientOtelMetricsProvider);


Should we pass noop here?

I don’t think we should do that. The xdsClientOtelMetricsProvider comes from the configuration and will always be either an XdsClientOtelTracker instance or a noop instance. Even if a null value is passed through constructor overloading, XdsClientJmx already handles that case internally and creates a new noop instance.

If that is the case, why in line 241 do we perform the check? Should we have that moved up before this line then?

That check is just for the backward compatibility. If XdsClientImpl constructor is called without XdsClientOtelMetricsProvider.

No, but i'm saying the usage of the actual parameter is a bit confusing though.. It is possible one value in line 239, then another after 241. We should just standardize that and have line 241 happen first.

Got it. Updated.

Kevin-Ngo · 2025-12-03T18:19:45Z

d2/src/main/java/com/linkedin/d2/xds/XdsClientImpl.java

    handleResourceRemoval(removedResources, type);
  }

  private void handleResourceUpdate(Map<String, ? extends ResourceUpdate> updates, ResourceType type)


Why does this method need to be updated? Do we need to handle null or empty string here?

I mainly needed to update the trackServerLatencyHelper function so that server latency can be recorded with the clientName, which will now be fetched using xdsClientJmx.getClientName(). While backtracking the flow, I found that the clientName can be extracted from the handleResourceUpdate function. I also added null and empty string checks in D2ClientJmxManager at the point where the client name is extracted and where the setClientName function is called.

Kevin-Ngo · 2025-12-03T18:20:59Z

d2/src/main/java/com/linkedin/d2/xds/XdsClientImpl.java

What makes some paths pass in NO_VALUE and others not?

Kevin-Ngo · 2025-12-03T18:21:14Z

d2/src/main/java/com/linkedin/d2/xds/balancer/XdsLoadBalancerWithFacilitiesFactory.java

        xdsStreamReadyTimeout,
        config.subscribeToUriGlobCollection,
        config._xdsServerMetricsProvider,
+        config._xdsClientOtelMetricsProvider,


We are missing tests for this

There are not any tests written for this file. Should I add a new test file for this?

yes, please do. Sometimes old files won't have it, though it's good practice to have coverage. If we're going through adding new features, this is a good time to add some coverage - at the very least to test our change.

Kevin-Ngo · 2025-12-03T18:22:08Z

d2/src/test/java/com/linkedin/d2/xds/TestXdsClientImpl.java

These tests seem to not do anything. Can we use a test impl of the XdsClientOtelMetricsProvider to verify the methods were called with the expected input variables?

aadityaraj7769 · 2025-12-04T10:27:57Z

Can we please add some context into the PR description around

What the Xds metrics are used for?

What are the new metrics now?

How was this tested?

Added in the PR description

Kevin-Ngo · 2025-12-08T18:42:34Z

Since this is OSS, we can't really link internal google docs. We should remove it from the PR description, same with references to internal things, such as container etc.

Kevin-Ngo · 2025-12-08T18:59:12Z

d2/src/main/java/com/linkedin/d2/jmx/D2ClientJmxManager.java

    final String jmxName = String.format("%s-XdsClientJmx", getGlobalPrefix(null));
+
+    // Get the client name from global prefix
+    String clientName = getGlobalPrefix(null);


One thing i've noticed also is that getGlobalPrefix is also called on line 308. Can we just call that method one time and reuse it?

Kevin-Ngo · 2025-12-08T19:10:18Z

d2/src/main/java/com/linkedin/d2/xds/balancer/XdsLoadBalancerWithFacilitiesFactory.java

        xdsStreamReadyTimeout,
        config.subscribeToUriGlobCollection,
        config._xdsServerMetricsProvider,
+        config._xdsClientOtelMetricsProvider,


yes, please do. Sometimes old files won't have it, though it's good practice to have coverage. If we're going through adding new features, this is a good time to add some coverage - at the very least to test our change.

Kevin-Ngo · 2025-12-08T19:12:24Z

d2/src/main/java/com/linkedin/d2/xds/XdsClientImpl.java

    }

-    _xdsClientJmx = new XdsClientJmx(serverMetricsProvider);
+    _xdsClientJmx = new XdsClientJmx(serverMetricsProvider, xdsClientOtelMetricsProvider);


If that is the case, why in line 241 do we perform the check? Should we have that moved up before this line then?

Kevin-Ngo · 2025-12-08T19:14:27Z

d2/src/test/java/com/linkedin/d2/jmx/XdsClientOtelMetricsProviderTest.java

Can we turn these tests into Parameterized ones? E.g.

@Test public void testRecordConnectionLost() { String clientName = "test-client-1"; _testProvider.recordConnectionLost(clientName); assertEquals(_testProvider.getCallCount("recordConnectionLost"), 1); assertEquals(_testProvider.getLastClientName("recordConnectionLost"), clientName); } @Test public void testRecordConnectionClosed() { String clientName = "test-client-2"; _testProvider.recordConnectionClosed(clientName); assertEquals(_testProvider.getCallCount("recordConnectionClosed"), 1); assertEquals(_testProvider.getLastClientName("recordConnectionClosed"), clientName); }

can be combined into a single parameterized test

aadityaraj7769 · 2025-12-09T06:09:11Z

Since this is OSS, we can't really link internal google docs. We should remove it from the PR description, same with references to internal things, such as container etc.

Updated.

Kevin-Ngo · 2025-12-16T23:10:50Z

d2/src/main/java/com/linkedin/d2/jmx/D2ClientJmxManager.java

+    // Get the client name from global prefix
+    String clientName = getGlobalPrefix(null);
+
+    final String jmxName = String.format("%s-XdsClientJmx", clientName);


nit: jmxName has nothing really to do with line:313-line:315, and could probably be declared after.

Kevin-Ngo · 2025-12-16T23:24:58Z

d2/src/main/java/com/linkedin/d2/jmx/XdsClientJmx.java

  private final AtomicInteger _resourceInvalidCount = new AtomicInteger();
  private final XdsServerMetricsProvider _xdsServerMetricsProvider;
+  private final XdsClientOtelMetricsProvider _xdsClientOtelMetricsProvider;
+


nit: unnecessary extra endline here. Additionally, we really should create a common util that exposes "-" in case future sensors will also need to reference it.

Removed extra endline and created new file D2JmxConstants which expose "-".

Kevin-Ngo · 2025-12-16T23:29:34Z

d2/src/main/java/com/linkedin/d2/jmx/XdsClientJmx.java

+   *
+   * @param clientName the name to identify this XDS client
+   */
+  public void setClientName(String clientName) {


nit: Is there a reason for this to be able to be set multiple times for a single instance? If not, we should have it be a one-way set. i.e. via atomic string or something.

Used atomic string.

Kevin-Ngo · 2025-12-16T23:31:05Z

d2/src/main/java/com/linkedin/d2/jmx/XdsClientJmx.java

    {
-      return _xdsClient.getActiveInitialWaitTimeMillis();
+      waitTime = _xdsClient.getActiveInitialWaitTimeMillis();
+      _xdsClientOtelMetricsProvider.updateActiveInitialWaitTime(_clientName, waitTime);


technically, can't all these usages of _clientName pass a null or empty _clientName if it's not set or improperly set?

Kevin-Ngo · 2025-12-16T23:33:01Z

d2/src/main/java/com/linkedin/d2/xds/XdsClientImpl.java

    }

-    _xdsClientJmx = new XdsClientJmx(serverMetricsProvider);
+    _xdsClientJmx = new XdsClientJmx(serverMetricsProvider, xdsClientOtelMetricsProvider);


No, but i'm saying the usage of the actual parameter is a bit confusing though.. It is possible one value in line 239, then another after 241. We should just standardize that and have line 241 happen first.

Kevin-Ngo · 2025-12-16T23:34:24Z

d2/src/test/java/com/linkedin/d2/xds/balancer/TestXdsLoadBalancerWithFacilitiesFactory.java

+/*
+   Copyright (c) 2023 LinkedIn Corp.
+
+   Licensed 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
+
+       http://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.
+*/


Is this necessary? This doesn't seem to be a standard across the files.

Kevin-Ngo · 2025-12-16T23:38:14Z

d2/src/test/java/com/linkedin/d2/xds/TestXdsClientImpl.java

    verify(fixture._wildcardResourceWatcher).onRemoval(CLUSTER_RESOURCE_NAME);
    verify(fixture._clusterSubscriber).onRemoval();
    verify(fixture._uriMapWildcardSubscriber).onRemoval(CLUSTER_RESOURCE_NAME);
    verifyZeroInteractions(fixture._serverMetricsProvider);


Should we verify our metrics aren't used here?

Kevin-Ngo · 2025-12-16T23:38:41Z

d2/src/test/java/com/linkedin/d2/xds/TestXdsClientImpl.java

    verify(fixture._resourceWatcher, times(0)).onChanged(D2_URI_MAP_GLOB_COLLECTION_UPDATE_WITH_DATA1);
    verify(fixture._wildcardResourceWatcher, times(0)).onChanged(any(),
        eq(D2_URI_MAP_GLOB_COLLECTION_UPDATE_WITH_DATA1));
    verifyZeroInteractions(fixture._serverMetricsProvider);


Basically anywhere that _serverMetricsProvider is used, since this is a dual implementation, we should have the same assertions to leverage the test cases.

Kevin-Ngo · 2025-12-17T19:38:40Z

d2/src/main/java/com/linkedin/d2/jmx/D2JmxConstants.java

+/*
+   Copyright (c) 2025 LinkedIn Corp.
+
+   Licensed 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
+
+       http://www.apache.org/licenses/LICENSE-2.0


also licensing here

Kevin-Ngo · 2025-12-17T19:39:39Z

d2/src/main/java/com/linkedin/d2/jmx/D2JmxConstants.java

+/**
+ * Common constants used across D2 JMX and metrics components.
+ */
+public final class D2JmxConstants


Should this be just a common utils file instead of D2JmxContants? The NO_VALUE will be used by possibly multiple sensors, so it should just be something like OpenTelemetryConstants.

Kevin-Ngo

Mostly LGTM (aside from minor improvement suggestions - which should be carried out) from Metrics standpoint. Melt ship - please have sensor owner review it to confirm it's capturing in the right places and has the correct testing.

adiraj_linkedin and others added 4 commits November 19, 2025 10:30

added the otel changes

7a43c50

Added more logic for the XdsClient Otel metrics

54a8401

Use NoOpXdsClientOtelMetricsProvider as default to eliminate null han…

0df69f4

…dling in XdsClientImpl

Update comments in XdsClientOtelMetricsProvider.java

ea5faa4

Kevin-Ngo reviewed Dec 3, 2025

View reviewed changes

Added tests for XdsClientOtelMetricsProvider

e9155b3

Kevin-Ngo reviewed Dec 8, 2025

View reviewed changes

Resolved PR comments

5cd8559

aadityaraj7769 requested a review from Kevin-Ngo December 9, 2025 13:48

Kevin-Ngo reviewed Dec 16, 2025

View reviewed changes

Resolved PR comments

f710f7e

Kevin-Ngo reviewed Dec 17, 2025

View reviewed changes

Kevin-Ngo approved these changes Dec 17, 2025

View reviewed changes

aadityaraj7769 force-pushed the adiraj/xdsclient-otel-migration branch from 512b8e2 to f710f7e Compare December 18, 2025 09:37

revert d2jmxconstant changes.

4effe1e

[WIP]: Add OpenTelemetry metrics support for XDS Client #1127

Are you sure you want to change the base?

[WIP]: Add OpenTelemetry metrics support for XDS Client #1127

Uh oh!

Conversation

aadityaraj7769 commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Backward Compatibility

About XdsSensor

New Otel Metrics

Counters

Histogram

Gauges

Uh oh!

Kevin-Ngo commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aadityaraj7769 commented Dec 4, 2025

Uh oh!

Kevin-Ngo commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aadityaraj7769 commented Nov 23, 2025 •

edited

Loading

Kevin-Ngo commented Dec 3, 2025 •

edited

Loading

Kevin-Ngo commented Dec 8, 2025 •

edited

Loading