Added support for HTTP/2 metrics to Netty. #1885
Conversation
millems
force-pushed
the
millem/netty-http2-metrics
branch
from
a6fe007 to
804e7bb
Compare
Updated existing HTTP metrics to specify that they are "concurrency", not "connections". For HTTP/2, we can't calculate the maximum number of connections, and concurrency is more useful to customers anyway. Fixed the AVAILABLE_CONCURRENCY in Netty to actually be the number of established but idle concurrency, not the difference between max concurrency and leased concurrency.
millems
force-pushed
the
millem/netty-http2-metrics
branch
from
804e7bb to
cf8eccb
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
| */ | ||
| public static final SdkMetric<Integer> MAX_CONNECTIONS = metric("MaxConnections", Integer.class); | ||
| public static final SdkMetric<Integer> MAX_CONCURRENCY = metric("MaxConcurrency", Integer.class); |
There was a problem hiding this comment.
Seems like for h2 operations, all metrics here are for streams, should we add metrics for connections?
There was a problem hiding this comment.
I'm not entirely confident that it's particularly useful to do so, but it wouldn't be difficult. Maybe we do it if we find it's needed? I'm worried about confusing people if we expose both.
There was a problem hiding this comment.
I think leasedConnection and availableConnection will provide helpful insights when we troubleshoot async issues. For example, in the past, we spent a lot of time searching logs and found out the streams are not balanced allocated across all connections. It'd save us time if we have such metrics available. That would require us to map connection metric with stream metric though.
| * The idle channel state for a specific channel. This should only be accessed from the {@link #executor}. | ||
| */ | ||
| private static final AttributeKey<ChannelIdleState> CHANNEL_STATE = | ||
| AttributeKey.newInstance("IdleConnectionCountingChannelPool.CHANNEL_STATE"); |
There was a problem hiding this comment.
should we namespace the key?
There was a problem hiding this comment.
I meant prefixing it with sdk namespace so it doesn't conflict with other keys. It seems we are using NettyUtils.getOrCreateAttributeKey now introduced in #1887
…76c5829c3 Pull request: release <- staging/9c3d9fb0-6453-4ef9-bb51-b1a76c5829c3
Updated existing HTTP metrics to specify that they are "concurrency", not "connections". For HTTP/2, we can't calculate the maximum number of connections, and concurrency is more useful to customers anyway.
Fixed the AVAILABLE_CONCURRENCY in Netty to actually be the number of established but idle concurrency, not the difference between max concurrency and leased concurrency.