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

Spikekill does not receive correct avgnan value when launching from GUI #3673

Closed
riversdev0 opened this issue Jul 9, 2020 · 7 comments · Fixed by #3685
Closed

Spikekill does not receive correct avgnan value when launching from GUI #3673

riversdev0 opened this issue Jul 9, 2020 · 7 comments · Fixed by #3685
Labels
bug Undesired behaviour resolved A fixed issue
Milestone

Comments

@riversdev0
Copy link
Contributor

Describe the bug

When launching Spikekill from the Cacti GUI, the incorrect value for avgnan (which is Replacement Method) is passed to the hook script if you choose either GapFill or Float. Choosing StdDev or Variance does not experience the issue.

To Reproduce

Steps to reproduce the behavior:

  1. Go to Cacti's Graph tab.

  2. Identify a graph upon which to run Spikekill. Click the Spikekill icon next to the graph to open the menu. Hover on Settings. Hover on Replacement Method. Chose Average.

  3. Click the Spikekill icon next to the graph again. Click 'Gap Fill Range'.

  4. Observe in the log/cacti.log file the messgage indicating avgnan is last:
    2020/07/08 18:21:18 - SPIKEKILL CactiUser:admin, File:eag-wan-rt02_traffic_in_72.rrd, Method:GapFill, OutStart:1594164011, OutEnd:1594250411, AvgNan:last

  5. Observe via a packet capture tool the HTTP request indicating that avgnan is last:
    image

Expected behavior

If the Replacement Method is set to 'Average', Spikekill should not be launched with 'Last Known Good', but rather with 'Average'.

Screenshots

Wireshark says the replacement is 'last'
image

The GUI says the replacement is 'average'
image

Desktop (please complete the following information)

n/a

Smartphone (please complete the following information)

n/a

Additional context

None

@riversdev0 riversdev0 added bug Undesired behaviour unverified Some days we don't have a clue labels Jul 9, 2020
@netniV
Copy link
Member

netniV commented Jul 9, 2020

seems logical to me, you're doing some good work on spikekill 👍

Will you be submitting a PR for this, or should I go ahead and fix it tomorrow?

@riversdev0
Copy link
Contributor Author

This one, actually, I have not found the cause for yet. I feel like it's in a Javascript somewhere, but I haven't spent enough time looking yet. If you know where the problem is, either go ahead and fix it, or tell me where to find it. Otherwise, I can search for it and fix it once I've located it. The proper solution should be to drop the parameter from the query string, because the script will pull the user's setting from the database, thus it is not even needed in the HTTP query.

@TheWitness
Copy link
Member

Everything is in layout.js. My thoughts here are that we should not even be passing in the parameter. When we do that, the spikekill.php should be getting the default from the database. So, we just have to remove "method=blah" from layout.js selectively.

@TheWitness
Copy link
Member

Here is a quick grep, you can see it's hardcoded.

image

@TheWitness
Copy link
Member

lol, it's 'avgnan'. But again, hardcoded. That's a bug. Let me do dig in this morning. I'll have to be a pull request.

@TheWitness TheWitness removed the unverified Some days we don't have a clue label Jul 12, 2020
@TheWitness TheWitness added this to the 1.2.13 milestone Jul 12, 2020
@TheWitness TheWitness linked a pull request Jul 12, 2020 that will close this issue
@TheWitness
Copy link
Member

Okay, got a good pull request now. Give it a look over. Some other things were wrong too, which @netniV will see strait away. This change will always pull the last stored value from the user settings, which was not happening before. So, if you changed something from the GUI and then expected it to just work, it would not. That's besides the fact that avgnan was always hardcoded.

@TheWitness TheWitness added the resolved A fixed issue label Jul 12, 2020
netniV pushed a commit that referenced this issue Jul 12, 2020
@riversdev0
Copy link
Contributor Author

Awesome, thank you!

@netniV netniV closed this as completed Jul 13, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Undesired behaviour resolved A fixed issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants