feat: Add S3 latency metrics #397
Conversation
| if (sensor != null) { | ||
| sensor.record(); | ||
| // metrics are reported per request, so 1 value can be assumed. | ||
| if (metricValues.size() == 1) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
API is not clear about it, but docs mention the following: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/metrics-list.html
Metrics collected with each request
So, my interpretation (and while testing this) is that each instance of metric collection corresponds to a single API call.
There was a problem hiding this comment.
Ok, I looked into the SDK code and seems you are right 👍
There was a problem hiding this comment.
But OTOH what harm would it do it it remains a loop? It'd be more future-proof and we wouldn't expect this internal behavior to change in a casual dependency upgrade
There was a problem hiding this comment.
The thing is that we will not be able to understand to what request type the latency metric belongs if there will be multiple values AFAIU. Otherwise I would agree.
There was a problem hiding this comment.
Oh, I see. That's very unfortunate. Well, let's rely on our tests as much as we can then
storage/s3/src/main/java/io/aiven/kafka/tieredstorage/storage/s3/MetricCollector.java
Show resolved
Hide resolved
| if (sensor != null) { | ||
| sensor.record(); | ||
| // metrics are reported per request, so 1 value can be assumed. | ||
| if (metricValues.size() == 1) { |
There was a problem hiding this comment.
Hm, I'm thinking that maybe it's worth logging a warning if the size is different from 1
There was a problem hiding this comment.
I was thinking the same actually, so yeah, lets have it.
There was a problem hiding this comment.
Make sense. Adding it now.
jeqo
force-pushed
the
jeqo/metrics-s3-latency
branch
from
86ec15f to
dd2c69f
Compare
| } | ||
|
|
||
| final var durations = metricCollection.metricValues(CoreMetric.API_CALL_DURATION); | ||
| if (durations.size() == 1) { |
There was a problem hiding this comment.
I think we need the same warning in the else here
jeqo
force-pushed
the
jeqo/metrics-s3-latency
branch
from
dd2c69f to
fcf1817
Compare
For better monitoring, latency metrics will help to understand how long are we waiting on the plugin side for a response from S3.
See commits for more details, a small refactor on the testing is included separately.