-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add S3DiskNoKeyErrors metric #66704
Add S3DiskNoKeyErrors metric #66704
Conversation
This is an automated comment for commit 34cba1b with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
Failing CI:
|
c407a02
to
2a893ed
Compare
src/IO/S3/Client.cpp
Outdated
/// The next call is NOT a recurcive call | ||
/// This is a virtuall call Aws::S3::S3Client::HeadObject(const Model::HeadObjectRequest&) | ||
if (isClientForDisk()) | ||
CurrentMetrics::add(CurrentMetrics::S3DiskNoKeyErrors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that you are counting here NoSuchKey
? it is not so obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we put such counters inside enrichErrorMessage
?
May be it would require rename the function. But in that case we would have one point when we catch the error code.
Co-authored-by: Sema Checherinda <[email protected]>
How could it be tested? |
All I can think of is an integration test where some object is removed from S3. But it looks like integration tests don't support removing objects from S3, so maybe you have something simpler in mind, @CheSema? |
It is possible to remove something or even all s3 data in integration tests. For example You could check your counter inside this test as well. Also please find some other tests which triggers some other S3 errors and check that your counter is 0. |
s3_disk_no_key_errors_metric_value = int( | ||
node.query( | ||
""" | ||
SELECT value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be sum(value)
otherwise you could have several integers values as a result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can only be one row in the result because there's only one row in system.metrics
where metric = 'S3DiskNoKeyErrors'
if I understood you correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
ac801e0
to
df3b1a0
Compare
df3b1a0
to
1d85f9b
Compare
b3d2ee8
to
092c837
Compare
3e752e1
to
7d45529
Compare
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add S3DiskNoKeyErrors metric
Documentation entry for user-facing changes
CI Settings (Only check the boxes if you know what you are doing):