collect connection pool metrics from aws #918
Conversation
| @@ -62,6 +62,9 @@ | |||
|
|
|||
| private static final Field[] COUNTERS = { | |||
| Field.BytesProcessed, | |||
| Field.HttpClientPoolAvailableCount, | |||
| Field.HttpClientPoolLeasedCount, | |||
| Field.HttpClientPoolPendingCount, | |||
There was a problem hiding this comment.
I think these would need to be recorded as gauges rather than counters. Per the docs they represent the current values for a given pool.
Spectator counters are used to measure a rate of change, e.g. bytes processed per second or requests per second. Gauges are used to snapshot a value at a given time, that seems more appropriate for something that is the number of connections currently in a given state for the pool.
There was a problem hiding this comment.
Totally agree. 83d916b attempts to address that.
There was a problem hiding this comment.
The gauge aspect looks fine with this change.
There was a problem hiding this comment.
My big concern here is it isn't clear how well a user can reason about the reported values. In particular, I think each client instance would have a separate connection pool. So if an app has N instances of say AmazonS3Client, then it would be somewhat arbitrary which state was reported.
since they represent current values per e.g. https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/util/AWSRequestMetrics.Field.html#HttpClientPoolAvailableCount
Totally agree here too, and thanks for pointing this out. I'm planning to stare at this and see if I can come up with some useful tagging strategy to make this work . Wanted to take the baby step of using gauges first, and once that's OK, move on to this. |
|
What do you think of choosing a key and looking for info in the handler context of the request? https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/AmazonWebServiceRequest.html#addHandlerContext-com.amazonaws.handlers.HandlerContextKey-X- |
dbyron-sf
force-pushed
the
add-connection-pool-counters
branch
from
e8a4d9e
to
6d7af1a
Compare
…h metrics from multiple clients
dbyron-sf
force-pushed
the
add-connection-pool-counters
branch
from
6d7af1a
to
e694c6b
Compare
|
How does e694c6b look? |
| */ | ||
| public SpectatorRequestMetricCollector(Registry registry, Map<String, String> customTags) { | ||
| this(registry, customTags, null); |
There was a problem hiding this comment.
@brharrington Actually as I stare at this it would be really helpful for all the bean wiring, etc. to have a default context key. What do you think of e.g. https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/handlers/HandlerContextKey.html#OPERATION_NAME?
There was a problem hiding this comment.
Here goes: c089bbb
There was a problem hiding this comment.
Operation name seems reasonable.
| @@ -156,6 +203,10 @@ private Id metricId(String metric, Map<String, String> tags) { | |||
| allTags.put(tag.getName(), tag.getValue(metrics).orElse(UNKNOWN)); | |||
| } | |||
| allTags.put(TAG_REQUEST_TYPE, request.getOriginalRequest().getClass().getSimpleName()); | |||
| String contextTagValue = request.getHandlerContext(handlerContextKey); | |||
| if (contextTagValue != null) { | |||
There was a problem hiding this comment.
Should the gauges be contingent on the context being set? Otherwise existing users might start getting the gauges for pool metrics and not realize that the values reported do not make sense when they have multiple pools in use.
There was a problem hiding this comment.
Yeah, that makes sense: 5b77464
|
Thanks much! |
No description provided.