otel: subchannel metrics #12202

AgraVator · 2025-07-07T06:18:40Z

Implements A94

core/src/main/java/io/grpc/internal/InternalSubchannel.java

kannanjgithub · 2025-07-23T03:25:01Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

@@ -415,7 +415,7 @@ void exitIdleMode() {
    LbHelperImpl lbHelper = new LbHelperImpl();
    lbHelper.lb = loadBalancerFactory.newLoadBalancer(lbHelper);
    // Delay setting lbHelper until fully initialized, since loadBalancerFactory is user code and
-    // may throw. We don't want to confuse our state, even if we will enter panic mode.
+    // may throw. We don't want to confuse our state, even if we enter panic mode.


I think the comment is more accurate as it is. After entering panic mode there is nothing much we can do about state, the comment implies delaying entering that panic mode and maintaining a sane state for the channel for as long as possible before bringing in potential user code.

Just a grammar change...

core/src/main/java/io/grpc/internal/InternalSubchannel.java

kannanjgithub · 2025-07-23T03:37:35Z

core/src/main/java/io/grpc/internal/InternalSubchannel.java

+                    addressIndex.getCurrentEagAttributes(), NameResolver.ATTR_BACKEND_SERVICE),
+                getAttributeOrDefault(
+                    addressIndex.getCurrentEagAttributes(), LoadBalancer.ATTR_LOCALITY_NAME),
+                "Peer Pressure",


This is not in the gRFC? Instead there is a "List of allowed values for grpc.disconnect_error".

For Phase 1 we won't be plumbing disconnect_error, will raise another PR with this as the base branch for the same

So? We can use a valid value now: unknown. Using "unknown" means we are technically implementing the gRFC with this change, and it is just an optimization to reduce the amount of unknowns.

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

core/src/main/java/io/grpc/internal/DelayedClientTransport.java

core/src/main/java/io/grpc/internal/InternalSubchannel.java

kannanjgithub · 2025-07-23T04:01:51Z

core/src/main/java/io/grpc/internal/MetricRecorderImpl.java

+   * @param optionalLabelValues the optional label values for the metric.
+   */
+  @Override
+  public void addLongUpDownCounter(LongUpDownCounterMetricInstrument metricInstrument, long value,


Add unit tests.

These are just wrappers on the underlying OTel API for UpDownCounter...

Right, but I do see logic here and there are still things to check. See that we have tests in MetricRecorderImplTest for addLongCounter(), which is very similar.

kannanjgithub · 2025-07-23T04:06:02Z

opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryMetricSink.java

@@ -117,6 +119,22 @@ public void addLongCounter(LongCounterMetricInstrument metricInstrument, long va
    counter.add(value, attributes);
  }

+  @Override
+  public void addLongUpDownCounter(LongUpDownCounterMetricInstrument metricInstrument, long value,


Add unit test.

added for the one above

What about in OpenTelemetryMetricsSinkTest for the updown metric ? Existing tests also check for metric enabled and disabled cases.

kannanjgithub · 2025-07-23T04:08:03Z

api/src/main/java/io/grpc/MetricInstrumentRegistry.java

+   * @return the newly created LongUpDownCounterMetricInstrument
+   * @throws IllegalStateException if a metric with the same name already exists
+   */
+  public LongUpDownCounterMetricInstrument registerLongUpDownCounter(String name,


Add unit test.

added for the one above

ejona86 · 2025-07-28T17:38:32Z

core/src/main/java/io/grpc/internal/MetricRecorderImpl.java

+   * @param optionalLabelValues the optional label values for the metric.
+   */
+  @Override
+  public void addLongUpDownCounter(LongUpDownCounterMetricInstrument metricInstrument, long value,


Right, but I do see logic here and there are still things to check. See that we have tests in MetricRecorderImplTest for addLongCounter(), which is very similar.

ejona86 · 2025-07-28T17:48:25Z

core/src/main/java/io/grpc/internal/InternalSubchannel.java

+                    addressIndex.getCurrentEagAttributes(), NameResolver.ATTR_BACKEND_SERVICE),
+                getAttributeOrDefault(
+                    addressIndex.getCurrentEagAttributes(), LoadBalancer.ATTR_LOCALITY_NAME),
+                "Peer Pressure",


So? We can use a valid value now: unknown. Using "unknown" means we are technically implementing the gRFC with this change, and it is just an optimization to reduce the amount of unknowns.

core/src/main/java/io/grpc/internal/OtelMetricsAttributes.java

core/src/main/java/io/grpc/internal/SubchannelMetrics.java

api/src/main/java/io/grpc/LoadBalancer.java

kannanjgithub

.

kannanjgithub · 2025-08-06T12:25:35Z

opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryMetricSink.java

@@ -117,6 +119,22 @@ public void addLongCounter(LongCounterMetricInstrument metricInstrument, long va
    counter.add(value, attributes);
  }

+  @Override
+  public void addLongUpDownCounter(LongUpDownCounterMetricInstrument metricInstrument, long value,


What about in OpenTelemetryMetricsSinkTest for the updown metric ? Existing tests also check for metric enabled and disabled cases.

ejona86 · 2025-08-19T23:15:19Z

core/src/main/java/io/grpc/internal/SubchannelMetrics.java

@@ -75,7 +76,7 @@ public SubchannelMetrics(MetricRecorder metricRecorder) {
    );
  }

-  public void recordConnectionAttemptSucceeded(OtelMetricsAttributes labelSet) {
+  public void recordConnectionAttemptSucceeded(MetricsAttributes labelSet) {


I suggest you consider inlining this class into InternalSubchannel. MetricsAttributes is awkward, as it isn't at all clear what values are used when. The caller is basically expected to know the implementation here, yet each method has at most two lines. We're not making anything simpler by adding this abstraction. Inlining these would seem to make things much more obvious. It's not like this has tests either; any change here will require a change to InternalSubchannelTest. I'd agree it is disappointing that registering the metrics is so long, but they aren't complicated lines. So complexity is low. (If we needed this API for real, I'd seriously consider having a separate class type passed into each of the methods.)

DisconnectError will end up needing to be public, in order to be used by transports. DisconnectError could be its own file, but really I'd put it in ManagedClientTransport, as it is really a part of the transportShutdown() API. But either way, this doesn't seem like a good place for it, as this class is just a helper for InternalSubchannel.

Thinking about it more, having this utility with just a ton of String arguments is really little different than the metric recorder API itself. In both cases we have to line up the arguments. So we could keep this utility class and just pass in a bunch of arguments instead of trying to package it in MetricsAttributes.

Thinking even more, we can sort of have our cake and eat it too without making a bunch of builders. If use the "lots o arguments" approach, but we put comments in the calling code (InternalSubchannel) for each argument, we can see that the argument names match. And ErrorProne will check that the comments match the variable names in SubchannelMetrics. See https://errorprone.info/bugpattern/ParameterName

(There's still the question of "how much value does the utility provide" and we could still choose to ditch it. But if you do want to keep it, I think this turns out better for this very specific problem at hand.)

ejona86 · 2025-08-19T23:35:33Z

api/src/main/java/io/grpc/EquivalentAddressGroup.java

+   * The name of the locality that this EquivalentAddressGroup is in.
+   */
+  public static final Attributes.Key<String> ATTR_LOCALITY_NAME =
+      Attributes.Key.create("io.grpc.lb.locality");


The string here is just for debugging. We typically try to match the API where the value is accessible. See ATTR_AUTHORITY_OVERRIDE just above, for example.

AgraVator marked this pull request as ready for review July 9, 2025 10:05

AgraVator requested review from ejona86 and kannanjgithub July 9, 2025 10:05

AgraVator force-pushed the otel-subchannel-metrics branch from eabbf58 to 9595507 Compare July 15, 2025 10:00

AgraVator commented Jul 16, 2025

View reviewed changes

core/src/main/java/io/grpc/internal/InternalSubchannel.java Outdated Show resolved Hide resolved

AgraVator added 8 commits July 22, 2025 23:25

otel: subchannel metrics

a06568c

otel: add LongUpDownCounterMetricInstrument

daf901b

otel: add LongUpDownCounterMetricInstrument

735665b

otel: plumb optional labels other than disconnect_error

a664b2f

otel: fixes NPE

2ce087e

otel: formatting fixes

2293e7f

add test case

623c0f9

logs the metrics inside sync context

c713561

AgraVator force-pushed the otel-subchannel-metrics branch from 9595507 to c713561 Compare July 22, 2025 17:55