Skip to content
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

Capture and parse the cmdstat timings from `INFO ALL` command #5926

Merged
merged 2 commits into from Aug 16, 2019

Conversation

@adamflott
Copy link
Contributor

adamflott commented May 30, 2019

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.
@glinton

This comment has been minimized.

Copy link
Member

glinton commented May 30, 2019

There is already an open PR for this in #5874. What do you feel this adds that that one doesn't provide?

@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented May 30, 2019

There is already an open PR for this in #5874. What do you feel this adds that that one doesn't provide?

Oops! Didn't see that. The only difference is, their PR does 2 remote calls, and mine does 1. Either way, I don't care which one is merged, just that it is :)

@wingyplus

This comment has been minimized.

Copy link
Contributor

wingyplus commented Jun 26, 2019

@adamflott your pr seems to be make sense than my opened pr. :)

plugins/inputs/redis/redis.go Outdated Show resolved Hide resolved
"usec": int64(990),
"usec_per_call": float64(990.0),
}
acc.AssertContainsTaggedFields(t, "redis_cmdstat", cmdstatCommandFields, cmdstatCommandTags)

This comment has been minimized.

Copy link
@wingyplus

wingyplus Jun 26, 2019

Contributor

Should we needs to add test in case that usec and usec_per_call are not number?

This comment has been minimized.

Copy link
@glinton

glinton Aug 5, 2019

Member

If this is something that may be returned, let's test for and handle it.

This comment has been minimized.

Copy link
@adamflott

adamflott Aug 15, 2019

Author Contributor

Is there a pattern from testutil I could use?

FWIW, I don't expect the type to change given the age of this commit :)
antirez/redis@d7ed7fd

@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Aug 4, 2019

@glinton any updates? I would like to see either #5874 or this PR get merged soon.

@glinton

This comment has been minimized.

Copy link
Member

glinton commented Aug 5, 2019

This likely won't make it into 1.12 as we're cramming trying to finish up what is already in the milestone. Because this is a basic change though, it is possible it gets into the 1.13 release Turns out this was in fact planned for 1.12, once the changes are made and this gets a final approval, it will be in the 1.12 release.

@glinton glinton added this to the 1.13.0 milestone Aug 5, 2019
plugins/inputs/redis/redis.go Outdated Show resolved Hide resolved
acc telegraf.Accumulator,
global_tags map[string]string,
) {
if strings.HasPrefix(name, "cmdstat") {

This comment has been minimized.

Copy link
@glinton

glinton Aug 5, 2019

Member

return if !strings.HasPrefix

This comment has been minimized.

Copy link
@adamflott

adamflott Aug 15, 2019

Author Contributor

done

"usec": int64(990),
"usec_per_call": float64(990.0),
}
acc.AssertContainsTaggedFields(t, "redis_cmdstat", cmdstatCommandFields, cmdstatCommandTags)

This comment has been minimized.

Copy link
@glinton

glinton Aug 5, 2019

Member

If this is something that may be returned, let's test for and handle it.

@glinton

This comment has been minimized.

Copy link
Member

glinton commented Aug 5, 2019

Resolves #2894
Preferred over and closes #2902

@adamflott can you also document the additional fields this adds, similar to 2902

@glinton glinton modified the milestones: 1.13.0, 1.12.0 Aug 5, 2019
@glinton glinton requested a review from danielnelson Aug 13, 2019
@glinton

This comment has been minimized.

Copy link
Member

glinton commented Aug 13, 2019

@adamflott not sure if you saw, but I was mistaken when i mentioned this would be pushed off until 1.13. Do you foresee yourself getting time to address the latest feedback?

@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Aug 13, 2019

@adamflott not sure if you saw, but I was mistaken when i mentioned this would be pushed off until 1.13. Do you foresee yourself getting time to address the latest feedback?

I can have this PR done by the end of tomorrow 8/14 EST.

I also have 2 other PRs around Redis I have vested interest in getting merged in

Any way I can get those reviewed?

acc telegraf.Accumulator,
global_tags map[string]string,
) {
if !strings.HasPrefix(name, "cmdstat") {

This comment has been minimized.

Copy link
@danielnelson

danielnelson Aug 15, 2019

Contributor

Nitpick, but I would add an underscore to match the TrimPrefix laster on:

- if !strings.HasPrefix(name, "cmdstat") {
+ if !strings.HasPrefix(name, "cmdstat_") {
@danielnelson

This comment has been minimized.

Copy link
Contributor

danielnelson commented Aug 15, 2019

The sentinel plugin will need to wait until 1.13, but the other should be able to make it.

@danielnelson danielnelson merged commit 96fa7fa into influxdata:master Aug 16, 2019
8 checks passed
8 checks passed
ci/circleci: deps Your tests passed on CircleCI!
Details
ci/circleci: package Your tests passed on CircleCI!
Details
ci/circleci: test-go-1.10 Your tests passed on CircleCI!
Details
ci/circleci: test-go-1.11 Your tests passed on CircleCI!
Details
ci/circleci: test-go-1.12 Your tests passed on CircleCI!
Details
ci/circleci: test-go-1.12-386 Your tests passed on CircleCI!
Details
ci/circleci: test-go-1.9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
bitcharmer added a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.