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

Change from INFO to INFO ALL to get Commandstats statistics) #2894 #2902

Closed
wants to merge 4 commits into from

Conversation

marek-knappe
Copy link

Added tests for #2894
Added documentation for #2894

Required for all PRs:

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

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a very minor thing that really shouldn't ever happen:

if section == "Commandstats" {
//#cmdstat_get:calls=63102137,usec=166854619,usec_per_call=2.64
tmp := strings.Split(line, ":")
tmp_metrics := strings.Split(tmp[1], ",")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for the length of the splits before indexing, just in case the input is bad for some reason. In the case of not having a ":", call acc.AddError and continue.

tmp := strings.Split(line, ":")
tmp_metrics := strings.Split(tmp[1], ",")
for _, element := range tmp_metrics {
mdata := strings.Split(element, "=")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check length here too

@danielnelson danielnelson added this to the 1.4.0 milestone Jun 9, 2017
for _, element := range tmp_metrics {
mdata := strings.Split(element, "=")
if ival, err := strconv.ParseInt(mdata[1], 10, 64); err == nil {
fields[tmp[0]+mdata[0]] = ival
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to separate tmp[0] & mdata[0] with a delimiter, such as _.

@danielnelson danielnelson modified the milestones: 1.4.0, 1.5.0 Aug 14, 2017
@danielnelson danielnelson added area/redis feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Aug 23, 2017
@danielnelson danielnelson modified the milestones: 1.5.0, 1.6.0 Nov 30, 2017
@russorat russorat modified the milestones: 1.6.0, 1.7.0 Jan 26, 2018
@danielnelson danielnelson modified the milestones: 1.7.0, 1.8.0 Jun 3, 2018
@glinton
Copy link
Contributor

glinton commented Aug 15, 2018

@marek-knappe do you have time soon to address the feedback from daniel and phemmer?

@russorat russorat modified the milestones: 1.8.0, 1.9.0 Sep 4, 2018
@danielnelson danielnelson modified the milestones: 1.9.0, 1.10 Oct 29, 2018
@russorat russorat modified the milestones: 1.10.0, 1.11.0 Jan 14, 2019
@danielnelson danielnelson modified the milestones: 1.11.0, 1.12.0 May 24, 2019
@glinton glinton removed this from the 1.12.0 milestone Aug 5, 2019
@sjwang90
Copy link
Contributor

Closing, addressed in PR #5926

@sjwang90 sjwang90 closed this Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/redis feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants