-
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.aerospike): Add latency info #15578
Conversation
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 contribution! I see there's a linter issue still present, could you resolve that and my preliminary comments? Thanks!
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you! |
Hi, Would be happy to circle back around on this PR after the above changes are made. Thanks! |
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you! |
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 making the changes!
@ajiraj2411 could you please fix the failing integration test which complains about "command latency not registered". This makes me think if your change won't break the plugin with older versions of the aerospike server? If so, please add a flag to the configuration to manually enable gathering the latency! |
@srebhan As per the doc - https://aerospike.com/docs/server/reference/info?version=7&all=true#latencies, this configuration available from 5.1.
Can you please help me with this? We can connect if you want tomorrow. |
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 🥳 This pull request decreases the Telegraf binary size by -6.47 % for linux amd64 (new size: 243.8 MB, nightly size 260.6 MB) 📦 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.
@ajiraj2411 thanks for your update! Some more small comments but we are close. Can I ask you for a favor? Could you please add an integration test with a newer version of an Aerospike container and actually collect the latency
metric!? You should be able to copy and adapt the existing test.
startTime, _ := time.Parse(time.TimeOnly, sTime) | ||
endTime, _ := time.Parse(time.TimeOnly, eTime) |
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.
Please check the returned errors as otherwise we may get strange results if the data provided by the API changes format or contains invalid values!
tests := []struct { | ||
name string | ||
args args | ||
want float64 |
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.
Please name the variable expected
.
if got := getDurationOfTransaction(tt.args.sTimeWithTimeZone, tt.args.eTime); got != tt.want { | ||
t.Errorf("getDurationOfTransaction() = %v, want %v", got, tt.want) | ||
} |
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.
Please use the corresponding require.Equal
function!
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you! |
Summary
Opening a feature request for Telegraf Aerospike Plugin with Latency Info
Checklist
Related issues
resolves #15579 which is duplicate of 4665