feat: Add S3 Metrics Collection #12213
Conversation
|
Hi @xin-zhang2! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
✅ Deploy Preview for meta-velox canceled.
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
There is a related PR here #12200 |
f0bcbf1 to
b7c3e59
Compare
|
|
||
| namespace facebook::velox::filesystems { | ||
|
|
||
| void registerS3Metrics(); |
There was a problem hiding this comment.
Let's add this to the public API registerS3FileSystem.h.
Prestissimo must call this similar to registerVeloxMetrics
There was a problem hiding this comment.
ok. Will move it to RegisterS3FileSystem.
| @@ -71,6 +73,8 @@ Aws::IOStreamFactory AwsWriteableStreamFactory(void* data, int64_t nbytes) { | |||
| return [=]() { return Aws::New<StringViewStream>("", data, nbytes); }; | |||
| } | |||
|
|
|||
| folly::once_flag s3MetricsFlag; | |||
There was a problem hiding this comment.
remove this since the client calls the registration.
There was a problem hiding this comment.
ok. I'll move the registration to the client side, and it will introduce changes in the Presto repo.
| "presto_cpp.hive.s3.list_status_calls"}; | ||
| constexpr folly::StringPiece kCounterS3ListLocatedStatusCalls{ | ||
| "presto_cpp.hive.s3.list_located_status_calls"}; | ||
| constexpr folly::StringPiece kCounterS3ListObjectsCalls{ |
There was a problem hiding this comment.
No. listStatusCalls, listLocatedStatusCalls and listObjectsCalls are not used because we don't call these APIs in Prestissimo. I keep them here just to make it consistent with the Presto Java. Do we need remove them?
| RECORD_METRIC_VALUE(kCounterS3GetObjectErrors); | ||
| } | ||
| RECORD_METRIC_VALUE(kCounterS3GetObjectRetries, outcome.GetRetryCount()); | ||
| RECORD_METRIC_VALUE(kCounterS3ActiveConnections, -1); |
There was a problem hiding this comment.
It follows the rule in Presto Java (https://github.com/prestodb/presto/blob/master/presto-hive/src/main/java/com/facebook/presto/hive/s3/PrestoS3FileSystem.java#L1031).
The value of activeConnections is increased by 1 before the read operation and decreased by 1 after the read is finished.
|
We also need to add a test. |
Add S3 file system metrics collection.