fix double metric registration across shared libraries (libgrpc/libgrpc++)#41910
fix double metric registration across shared libraries (libgrpc/libgrpc++)#41910h-vetinari wants to merge 2 commits intogrpc:masterfrom
Conversation
…pc++) ResourceQuotaDomain's 5 metrics were defined as `static inline` data members in telemetry.h. Each DSO that transitively includes this header (via memory_quota.h) gets its own copy of the initializers, causing duplicate registration in InstrumentIndex when both libgrpc.so and libgrpc++.so are loaded — even though libgrpc++ never uses the metrics. Convert to function-local statics (Meyers singletons), which are only initialized on first call. Since libgrpc++ never calls these methods, no registration occurs there. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the resource quota telemetry metric registration in gRPC. It changes the metric handles from static inline data members to function-local statics (Meyers singletons) to ensure lazy initialization and prevent duplicate metric registration issues when headers are included in different shared libraries. The change affects kCallsDropped, kCallsRejected, kConnectionsDropped, kInstantaneousMemoryPressure, and kMemoryPressureControlValue, which are now accessed via new accessor functions. A review comment suggests improving the naming of these new accessor functions by removing the Num and Dbl prefixes for better conciseness and consistency.
| static const auto& NumCallsDropped() { | ||
| static const auto handle = RegisterCounter( | ||
| "grpc.resource_quota.calls_dropped", | ||
| "EXPERIMENTAL. Number of calls dropped due to resource quota " | ||
| "exceeded", | ||
| "calls"); | ||
| return handle; | ||
| } | ||
| static const auto& NumCallsRejected() { | ||
| static const auto handle = RegisterCounter( | ||
| "grpc.resource_quota.calls_rejected", | ||
| "EXPERIMENTAL. Number of calls rejected due to resource quota " | ||
| "exceeded", | ||
| "calls"); | ||
| return handle; | ||
| } | ||
| static const auto& NumConnectionsDropped() { | ||
| static const auto handle = RegisterCounter( | ||
| "grpc.resource_quota.connections_dropped", | ||
| "EXPERIMENTAL. Number of connections dropped due to resource quota " | ||
| "exceeded", | ||
| "connections"); | ||
| return handle; | ||
| } | ||
| static const auto& DblInstantaneousMemoryPressure() { | ||
| static const auto handle = RegisterDoubleGauge( | ||
| "grpc.resource_quota.instantaneous_memory_pressure", | ||
| "The current instantaneously measured memory pressure.", "ratio"); | ||
| return handle; | ||
| } | ||
| static const auto& DblMemoryPressureControlValue() { | ||
| static const auto handle = RegisterDoubleGauge( | ||
| "grpc.resource_quota.memory_pressure_control_value", | ||
| "A control value that can be used to scale buffer sizes up or down to " | ||
| "adjust memory pressure to our target set point.", | ||
| "ratio"); | ||
| return handle; | ||
| } |
There was a problem hiding this comment.
While the Num and Dbl prefixes are descriptive of the metric types, they make the function names a bit verbose and less consistent with the original metric names (e.g., kCallsDropped). The Dbl prefix in particular can be slightly awkward to read.
Consider removing these prefixes for cleaner and more concise accessor functions. The type of the metric is already clear from the context where these handles are used (e.g., Increment for counters, Set for gauges), and the compiler would enforce correct usage.
Suggested renames:
NumCallsDropped()->CallsDropped()NumCallsRejected()->CallsRejected()NumConnectionsDropped()->ConnectionsDropped()DblInstantaneousMemoryPressure()->InstantaneousMemoryPressure()DblMemoryPressureControlValue()->MemoryPressureControlValue()
This would make the call sites cleaner. If you agree with this suggestion, please apply it to all the new accessor functions in this file and update their call sites in other files accordingly.
There was a problem hiding this comment.
@tanvi-jagtap (or anyone who has an opinion): can you let me know which naming you prefer? I chose NumCallsDropped (not least because CallsDropped could be interpreted as something else, e.g. a boolean), and correspondingly used Dbl... for the constants that aren't integers. Happy to change the naming as gemini suggests, but please confirm that this is your preferred variant.
Not sure how to test this, short of doing the kind of everything-as-a-shared-library build in CI here. Beyond that, even the basic mechanics are still under discussion - @markdroth commented in the issue that he still suspects an ODR issue. I can find no evidence for that though. Please check out the commit message in db71f4d for my understanding of the situation. |
|
I'd like @ctiller to weigh in here. He's a lot more familiar with low-level compiler behavior and can probably better understand this issue and recommend the right approach. |
|
Ping @ctiller |
|
Ping @ctiller :) |
Fixes #41672