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.

Sign up for GitHub

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

Jump to bottom

nsqadmin: make "rate" column work without --proxy-graphite #734

Merged
merged 2 commits into from
Apr 9, 2016
Merged

nsqadmin: make "rate" column work without --proxy-graphite #734

merged 2 commits into from
Apr 9, 2016

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Apr 8, 2016

graphite /render requests for "rate" would always be proxied through nsqadmin, even without --proxy-graphite enabled. I figured out a way to make it work more like the graphite images do.

I imagine this won't win any frontend code-style awards, and welcome comments related to that.

I left the /graphite handler in nsqadmin in case someone has custom frontend templates they want to continue using, but do you think I should remove it?

ploxiln added 2 commits April 8, 2016 10:59
@ploxiln
nsqadmin: add dependency on gulp-task-listing
c88fba5 
seems to need it to gulp build
@ploxiln
nsqadmin: make "rate" column work without --proxy-graphite
0c353f4 
Requests to graphite /render endpoint for "rate" column would always
be proxied. Further, when not proxied, they need to use "jsonp" to
avoid CORS problems.
@mreiferson
Copy link
Member

got sshots to prove this works

wayyyyyyyyyyy to much javascript to review on a friday night man

@mreiferson
Copy link
Member

❤️

@mreiferson mreiferson added the bug label Apr 9, 2016
@ploxiln
Copy link
Member Author

ploxiln commented Apr 9, 2016

sure.
btw "dockerdev", "nsqadmin.dockerdev", "graphite.dockerdev" are my dev VM

before

screen shot 2016-04-09 at 00 15 46

nsqadmin log

[nsqadmin] 2016/04/09 04:15:36.308407 GRAPHITE: http://graphite.dockerdev/render?format=json&from=-120sec&target=sumSeries%28nsq.%2A.topic.events.message_count%29&until=-60sec
[nsqadmin] 2016/04/09 04:15:36.308931 ERROR: graphite request failed - got response 401 Unauthorized "<html>\r\n<head><title>401 Authorization Required</title></head>\r\n<body bgcolor=\"white\">\r\n<center><h1>401 Authorization Required</h1></center>\r\n<hr><center>nginx</center>\r\n</body>\r\n</html>\r\n"

after

screen shot 2016-04-09 at 00 19 00

browser request

GET /render?jsonp=jQuery223032047556238993613_1460175537694&target=sumSeries(nsq.*.topic.events.message_count)&from=-120sec&until=-60sec&format=json&_=1460175537695 HTTP/1.1
Host: graphite.dockerdev
...
Referer: http://nsqadmin.dockerdev/
Authorization: Basic *****
...

HTTP/1.1 200 OK
Server: nginx
Date: Sat, 09 Apr 2016 04:18:58 GMT
Content-Type: text/javascript
Transfer-Encoding: chunked
...

jQuery223032047556238993613_1460175537694([{"target": "sumSeries(nsq.*.topic.events.message_count)", "datapoints": [[2.0, 1460175420]]}])

with --proxy-graphite

well, same screenshot, it works

browser request

GET /render?jsonp=jQuery22309552545633412136_1460177590870&target=sumSeries(nsq.*.topic.events.message_count)&from=-120sec&until=-60sec&format=json&_=1460177590871 HTTP/1.1
Host: nsqadmin.dockerdev
...
X-UserAgent: nsqadmin/v0.3.7
X-Requested-With: XMLHttpRequest
Referer: http://nsqadmin.dockerdev/
Authorization: Basic *****
...

HTTP/1.1 200 OK
Server: nginx
Date: Sat, 09 Apr 2016 04:53:11 GMT
Content-Type: text/javascript
Content-Length: 147
...

jQuery22309552545633412136_1460177590870([{"target": "sumSeries(nsq.*.topic.events.message_count)", "datapoints": [[2.0, 1460177520]]}])

It's worth noting that there are zero log messages in nsqadmin for these proxied requests, and also they are sent without changing the Host header, so to actually make it work I had to e.g. change --graphite-url to http://localhost:9090 to bypass the nginx subdomain proxying setup I've got here. Anyway, it doesn't make sense to add a bunch of options to support setups besides connections directly to graphite-web for this case, just clarifying the tested config.

@mreiferson
Copy link
Member

LGTM, thanks plo

@mreiferson mreiferson merged commit 9156b4b into nsqio:master Apr 9, 2016
@ploxiln ploxiln mentioned this pull request Apr 24, 2016
Open
@ploxiln ploxiln deleted the graphite_rate_noproxy branch December 29, 2016 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ploxiln @mreiferson