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

nsqadmin: fix graphite 'target' param serialization #1498

Merged

Conversation

johnou
Copy link
Contributor

@johnou johnou commented Oct 3, 2024

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]

The target param can also be repeated to graph multiple related metrics.

&target=company.server1.loadAvg&target=company.server1.memUsage

with the current array param our graphite server is responding with

{"errors": {"target": "This parameter is required."}}

removing the array from the param fixes the graphs.

[1] https://graphite.readthedocs.io/en/latest/render_api.html#target

@johnou johnou changed the title Correct graphite 'target' param serialization with traditional $.param nsqadmin: correct graphite 'target' param serialization with traditional $.param Oct 3, 2024
@johnou johnou changed the title nsqadmin: correct graphite 'target' param serialization with traditional $.param nsqadmin: fix graphite 'target' param serialization with traditional $.param Oct 3, 2024
@johnou johnou changed the title nsqadmin: fix graphite 'target' param serialization with traditional $.param nsqadmin: fix graphite 'target' param serialization Oct 3, 2024
@johnou johnou force-pushed the bugfix/nsqadmin_graphite_target_param branch from af13dde to 70a5c77 Compare October 3, 2024 13:37
@johnou
Copy link
Contributor Author

johnou commented Oct 4, 2024

@mreiferson ptal, fwiw I would be more comfortable with you rebuilding the minified javascript.. :) it feels wrong submitting "obfuscated" code.

@mreiferson
Copy link
Member

Thanks for this. Yes, I'll build the JS if you want to update this PR to just have the actual fix.

@mreiferson mreiferson added the bug label Oct 6, 2024
@johnou johnou force-pushed the bugfix/nsqadmin_graphite_target_param branch from 70a5c77 to fe913a0 Compare October 7, 2024 08:51
@johnou
Copy link
Contributor Author

johnou commented Oct 7, 2024

@mreiferson thanks, updated!

@johnou johnou force-pushed the bugfix/nsqadmin_graphite_target_param branch from fe913a0 to 09c645d Compare October 10, 2024 06:20
@johnou
Copy link
Contributor Author

johnou commented Oct 10, 2024

@mreiferson ptal I reverted the readability / stylistic changes too.

@johnou
Copy link
Contributor Author

johnou commented Oct 10, 2024

fwiw this regression was caused by #1062 specifically upgrading jquery.. "As of jQuery 3.0, the $.param() method no longer uses jQuery.ajaxSettings.traditional as its default setting and will default to false. For best compatibility across versions, call $.param() with an explicit value for the second argument and do not use defaults." [1]

[1] https://api.jquery.com/jQuery.param/#jQuery-param-obj-traditional

@johnou
Copy link
Contributor Author

johnou commented Oct 17, 2024

@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.

@mreiferson
Copy link
Member

LGTM

@mreiferson mreiferson merged commit 73d42c4 into nsqio:master Nov 18, 2024
@mreiferson mreiferson mentioned this pull request Nov 18, 2024
@johnou
Copy link
Contributor Author

johnou commented Nov 18, 2024

thanks!

@johnou johnou deleted the bugfix/nsqadmin_graphite_target_param branch November 18, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants