Conversation
There was a problem hiding this comment.
Can you review go/java-practices/null from the overall PR's perspective in terms of nullable vs optional ? I am not sure if we have a local java convention to prefer or avoid null
| private final int maxMessageSize; | ||
| private final int maxHeaderListSize; | ||
| private final int softLimitHeaderListSize; | ||
| private MetricRecorder metricRecorder; |
There was a problem hiding this comment.
How is this being set? I don't see a constructor or a setter that accesses this value?
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| final class TcpMetrics { |
There was a problem hiding this comment.
optional: Javadocs for classes might be helpful
| } | ||
|
|
||
| static final class Metrics { | ||
| final LongCounterMetricInstrument connectionsCreated; |
There was a problem hiding this comment.
nullability annotations for fields where needed. Also consider the previous discussion about nullability style guide
| * Safe metric registration or retrieval for environments where TcpMetrics might | ||
| * be loaded multiple times (e.g., shaded and unshaded). | ||
| */ | ||
| private static LongCounterMetricInstrument safelyRegisterLongCounter( |
There was a problem hiding this comment.
Do we need the safelyRegi... private abstraction?
Given that all registration happens during construction and this class is finel , can't we simply ensure we dedupe our metrics during construction, catch and rethrow the error from the constructor? i.e. ensure that the input is valid and catch/throw from a single point which should never be reached.
The fact that "we are passing a valid set of metrics for registration" can probably be trivially unit tested when we construct a TcpMetrics instance.
This should reduce code bloat without any sacrifice to safety, but at the cost of "duplicate input will throw an error instead of being handled intuitively", which is a fair expectation IMO.
| java.util.List<String> labelValues = getLabelValues(channel); | ||
| try { | ||
| if (channel.getClass().getName().equals(epollSocketChannelClassName)) { | ||
| Class<?> tcpInfoClass = Class.forName(epollTcpInfoClassName); |
There was a problem hiding this comment.
This is quite a lot of reflection in hot exporting path. So, a couple of questions.
- Why do we choose reflectin over typecasting and calling actual methods? Reducing dependency surface?
- Is there a way to optimise this like caching value on per channel level? Something like a creating a runnable per channel. Not sure how it'd be possible to share this data for the final collection on channel inactive.
| Method rttMethod = tcpInfoClass.getMethod("rtt"); | ||
|
|
||
| long totalRetrans = (Long) totalRetransMethod.invoke(info); | ||
| int retransmits = (Integer) retransmitsMethod.invoke(info); |
There was a problem hiding this comment.
Are these cumulative metrics or are these total retransmits since the connection start? If latter, the current logic is incorrect.
There was a problem hiding this comment.
AAh I see, then I think we should not be using a counter for this... Will clarify in the gRFC
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
"otel" is a very surprising prefix to use for the PR/commit, as opentelemetry isn't actually changed at all.
| * @since 1.81.0 | ||
| */ | ||
| @CanIgnoreReturnValue | ||
| public NettyServerBuilder setMetricRecorder( |
There was a problem hiding this comment.
MetricRecorder is @Internal, so this would need to be internal. On client-side we have addMetricSink(); we always use MetricRecorderImpl. Why do that differently here? This approach won't allow having multiple MetricSinks. Although I really don't understand what is going on, since ServerImplBuilder has addMetricSink().
| @Nullable | ||
| public MetricRecorder getMetricRecorder() { | ||
| return metricRecorder; | ||
| } |
There was a problem hiding this comment.
LoadBalancer.Helper.getMetricRecorder() is non-null and ManagedChannelImpl creates a MetricRecorderImpl unconditionally. Make this non-null as well? I see that NameResolver allows it to be null; seems we should just change that. LoadBalancer used to be that way and it was changed after a bug in the Helper implementation caused the channel to panic.
If we need the optimization, we can have the MetricRecorder return whether a particular instrument is enabled. But I see no need to add an additional condition to usages for the metric recorder being missing entirely.
| * Sets the MetricRecorder for the server. This optional method allows setting | ||
| * the MetricRecorder after construction but before start(). | ||
| */ | ||
| default void setMetricRecorder(MetricRecorder metricRecorder) {} |
There was a problem hiding this comment.
I want to avoid setters-before-start for this API, as they get wonky and prevent using final. Let's just construct it within the builder and pass it as an argument to ServerImplBuilder.ClientTransportServersBuilder.buildTransportServers()? I'd agree that might not be how it is done long-term, but it is probably closer to the final form than a setter. Right now ServerImpl doesn't even need MetricRecorder itself.
| optionalLabels); | ||
|
|
||
| if (epollAvailable) { | ||
| packetsRetransmitted = safelyRegisterLongCounter(registry, |
There was a problem hiding this comment.
The expectation was that instruments would all be registered unconditionally.
| try { | ||
| return registry.registerLongCounter(name, description, unit, requiredLabelKeys, | ||
| optionalLabelKeys, false); | ||
| } catch (IllegalStateException e) { |
There was a problem hiding this comment.
No, no, no. IllegalStateException means "you, the programmer, messed up and should not have called in this state." It is a bug. Prevent the exception from happening; don't try to clean up afterward.
Since these metrics are shared across transports, the registrations need to be as well. I'd say we should put it in grpc-core like done for SubchannelMetrics, but I realize now that would be broken if we shade grpc-core into transports. So these really need to be defined in grpc-api, although they can be in an Internal* class. (We'll need to fix SubchannelMetrics as well.)
| this(metricRecorder, target, DEFAULT_METRICS); | ||
| } | ||
|
|
||
| Tracker(MetricRecorder metricRecorder, String target, Metrics metrics) { |
| "io.netty.channel.epoll.EpollTcpInfo"); | ||
| } | ||
|
|
||
| Tracker(MetricRecorder metricRecorder, String target, Metrics metrics, |
| private io.netty.util.concurrent.ScheduledFuture<?> reportTimer; | ||
|
|
||
| void channelActive(Channel channel) { | ||
| if (metricRecorder != null && target != null) { |
There was a problem hiding this comment.
When would the target be null, and if it is null why would we stop metrics? Seems we should worst-case use "" or something of the style <unknown target>. Otherwise you're silently losing metrics, which is the worst failure model for metrics as then you have lies.
| Collections.singletonList(target), labelValues); | ||
| } | ||
| } catch (Throwable t) { | ||
| // Epoll not available or error getting tcp_info, just ignore. |
There was a problem hiding this comment.
This comment only applies to a portion of the try block. Limit the scope of the try block to just where it is needed.
| return; | ||
| } | ||
| synchronized (accessorCache) { | ||
| channelReflectionAccessor = accessorCache.get(epollTcpInfoClassName); |
There was a problem hiding this comment.
Please no specialized cache just for testing. I'd suggest a single field and if epollTcpInfoClassName is provided for testing then simply create a new one (and don't cache it). And we can do all that in the constructor instead of checking every invocation. We do have some "interesting" tools that might help testing this (e.g., StaticTestingClassLoader), but I'll have to look over the tests to see how much help it provides.
3 participants
No description provided.