-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat(inputs.ceph): Use perf schema to determine metric type #15233
Conversation
6216e7a
to
9373440
Compare
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.
Thanks @awojno-bloomberg for your contribution! I have some comments regarding handling of errors in the code...
3ab8071
to
2f817b6
Compare
Hi, thanks for taking a look at this PR. I am new to Go/Telegraf so I appreciate all of the comments. Please let me know if there is anything I can do for this PR. |
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.
Thanks for the nice update @awojno-bloomberg!
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.
Hi,
Thanks for the PR! Can you please add some tests that cover the parseSchema function?
As far as backwards compatibility, it does look like all the recent versions of Ceph have supported the ability to dump the performance schema. I did have a concern about printing the warning out at every collection interval, if older versions did not.
Thanks!
2f817b6
to
383fb48
Compare
Thanks for taking a look. I added a test case for parseSchema. Let me know if anything else is needed there. For the second point, because performance schema has been supported for a while, should I change it so that instead of the warning, I instead add a message to acc.AddError and |
383fb48
to
6137442
Compare
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Thanks for taking a look. I added a test case for parseSchema. Let me know if anything else is needed there.
Thanks!
For the second point, because performance schema has been supported for a while, should I change it so that instead of the warning, I instead add a message to acc.AddError and continue similar to what happens when perfDump errors?
I generally prefer we keep things as backwards compatible as possible in case someone somewhere still has a version that doesn't support it or the format is different. Let's leave it as-is with the warning for now.
Thanks again for the PR!
Sounds good to me! What would be the next steps to get this PR merged? |
@DStrand1 is up to review and if he is happy, then he gets to click merge. |
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.
Looks great to me, thanks!
Summary
Run ceph perf schema to determine the metric types and use that information to correctly parse metrics.
Checklist
Related issues
resolves #