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: --proxy-graphite option is broken #1287

Merged
merged 5 commits into from
Oct 19, 2020
Merged

Conversation

mreiferson
Copy link
Member

  • nsqadmin v1.2.0 (built w/go1.12.9)
  • cmd works:
    • ./bin/nsqadmin --lookupd-http-address=${localip}:4161 -graphite-url=http://${localip}:80
  • cmd cause whole site down:
    • ./bin/nsqadmin --lookupd-http-address=${localip}:4161 -graphite-url=http://${localip}:80 -proxy-graphite

there is a js error in browser:

  • Uncaught SyntaxError: Unexpected token ';'

index page shows:

<script type="text/javascript">
        var USER_AGENT = 'nsqadmin/v1.2.0';
        var VERSION = "1.2.0";
        var GRAPHITE_URL = ;
        var GRAPH_ENABLED = true;
        var STATSD_COUNTER_FORMAT = "stats.counters.%s.count";
        var STATSD_GAUGE_FORMAT = "stats.gauges.%s";
        var STATSD_INTERVAL =  60 ;
        var STATSD_PREFIX = "nsq.%s";
        var NSQLOOKUPD = ["127.0.0.1:4161",];
        var IS_ADMIN =  true ;
        var BASE_PATH = "/";
    </script>

the GRAPHITE_URL is empty

@ploxiln ploxiln added the bug label Oct 10, 2020
@mreiferson mreiferson changed the title nsqadmin "--proxy-grahpite" option causes site down nsqadmin: --proxy-graphite option is broken Oct 11, 2020
@mreiferson
Copy link
Member

mreiferson commented Oct 11, 2020

Interesting... I used hub to convert this issue to a PR, and it looks like I became the original author????????

(credit for this issue goes to @fanlix)

@mreiferson mreiferson force-pushed the nsqadmin-fix-graphite-url branch from a2e09f4 to 80cff45 Compare October 11, 2020 04:56
@mreiferson
Copy link
Member

Anyway, what a fun Saturday night! This updates nsqadmin static dependencies (e.g. Gulp) to work with Node 14.x, updates gulpfile.js, and rebuilds static assets using the maintained version of go-bindata (all to pickup this two character fix
17f69f1).

WARNING: 🟥 I haven't tested any of this... 🔴

@ploxiln
Copy link
Member

ploxiln commented Oct 12, 2020

I can test the non --proxy-graphite case, hopefully soon ...

@fanlix can you compile nsqadmin from this branch and try it for your --proxy-graphite setup?

@fanlix
Copy link
Author

fanlix commented Oct 12, 2020

@ploxiln ok. I will do it today.

@fanlix
Copy link
Author

fanlix commented Oct 12, 2020

bad news, this patch not work.

  • complie by: CGO_ENABLED=0 make BLDFLAGS='-ldflags="-s -w"' nsqadmin
  • run with graphite options, fail
  • run without graphite options, fail: ./nsqadmin --lookupd-http-address=127.0.0.1:4161

nsqadmin access logs looks ok

[nsqadmin] 2020/10/12 12:50:52.884786 INFO: nsqadmin v1.2.1-alpha (built w/go1.14.2)
[nsqadmin] 2020/10/12 12:50:52.885118 INFO: HTTP: listening on [::]:4171
[nsqadmin] 2020/10/12 12:50:55.666282 INFO: 200 GET / (xxxx:23251) 89.628µs
[nsqadmin] 2020/10/12 12:50:56.731998 INFO: 200 GET / (xxxx:23251) 39.213µs

but browser gets blank page

curl -i http://xxxx:4171/

HTTP/1.1 200 OK
Content-Type: text/html
Date: Mon, 12 Oct 2020 04:57:30 GMT
Content-Length: 0

the 2-char modify in index.html is the right fix

  • I modify index.html (release version) in browser-dev-tool
    • add that 2-char by hand.
    • it works for graphite and proxy
  • but gulp/binddata hard to handle. :(

@ploxiln
Copy link
Member

ploxiln commented Oct 12, 2020

yeah, the bindata which mreiferson generated seems to be messed up, I also get blank pages:

$ curl -v localhost:4171/topics
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 4171 (#0)
> GET /topics HTTP/1.1
> Host: localhost:4171
> User-Agent: curl/7.64.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< Content-Type: text/html
< Date: Mon, 12 Oct 2020 05:42:30 GMT
< Content-Length: 0
< 
* Connection #0 to host localhost left intact
* Closing connection 0

@ploxiln
Copy link
Member

ploxiln commented Oct 12, 2020

the build of go-bindata I happen to have installed seems to work better, I went ahead pushed up the result to this branch

@ploxiln
Copy link
Member

ploxiln commented Oct 12, 2020

(also it seems travis-ci results aren't being posted here, but the CI is running and can be seen at https://travis-ci.org/github/nsqio/nsq/pull_requests)

@mreiferson
Copy link
Member

What do you mean by “installed”? The question I have is whether it’s the version of go-bindata I’m using or node/gulp/my changes not producing correct static assets as input.

I’ll check locally tomorrow (replying on mobile)

@ploxiln
Copy link
Member

ploxiln commented Oct 12, 2020

I just mean I built this go-bindata and put it in my PATH about a year ago:

$ ls -l ~/go/bin/go-bindata 
-rwxr-xr-x  1 pierce  staff  2882872 Sep 11  2019 /Users/pierce/go/bin/go-bindata
$ go-bindata --version
go-bindata 3.1.3 (Go runtime go1.12.9).
Copyright (c) 2010-2013, Jim Teeuwen.

I don't think that singularly narrows down the fork I used, but I think it's older than the suggested fork you put in the README ...

@mreiferson mreiferson force-pushed the nsqadmin-fix-graphite-url branch 2 times, most recently from 7ba7307 to def011f Compare October 13, 2020 03:35
@mreiferson
Copy link
Member

The latest version of go-bindata that I used changed the behavior of the --prefix argument, requiring a trailing / in order to produce the right keys in the _bindata map.

@ploxiln
Copy link
Member

ploxiln commented Oct 13, 2020

cool, I'll test it out

@fanlix
Copy link
Author

fanlix commented Oct 13, 2020

by my test it works, for both proxy and un-proxy. 👍

Copy link
Member

@ploxiln ploxiln left a comment

Choose a reason for hiding this comment

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

All looks good and works fine for me. Should probably just squash commits a bit.

@mreiferson mreiferson force-pushed the nsqadmin-fix-graphite-url branch from def011f to 58ab1ec Compare October 18, 2020 18:34
@mreiferson
Copy link
Member

fixed up the commits

@jehiah jehiah merged commit e989f96 into master Oct 19, 2020
@jehiah jehiah deleted the nsqadmin-fix-graphite-url branch October 19, 2020 00:46
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.

4 participants