-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
nsqadmin: fix graphite 'target' param serialization #1498
nsqadmin: fix graphite 'target' param serialization #1498
Conversation
af13dde
to
70a5c77
Compare
@mreiferson ptal, fwiw I would be more comfortable with you rebuilding the minified javascript.. :) it feels wrong submitting "obfuscated" code. |
Thanks for this. Yes, I'll build the JS if you want to update this PR to just have the actual fix. |
70a5c77
to
fe913a0
Compare
@mreiferson thanks, updated! |
fe913a0
to
09c645d
Compare
@mreiferson ptal I reverted the readability / stylistic changes too. |
fwiw this regression was caused by #1062 specifically upgrading jquery.. "As of jQuery 3.0, the [1] https://api.jquery.com/jQuery.param/#jQuery-param-obj-traditional |
@mreiferson let me know if you are happy with using the traditional param, we could also change the way the params are built but it's probably not worth the effort. |
LGTM |
thanks! |
Changed the
$.param()
calls to$.param(q, true)
to ensure that arrays are serialized using traditional style, resulting in multiple target parameters instead of target[].From graphite docs [1]
with the current array param our graphite server is responding with
removing the array from the param fixes the graphs.
[1] https://graphite.readthedocs.io/en/latest/render_api.html#target