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

nsq*: log HTTP requests #610

Merged
merged 1 commit into from
Jul 28, 2015
Merged

nsq*: log HTTP requests #610

merged 1 commit into from
Jul 28, 2015

Conversation

mreiferson
Copy link
Member

This adds a log decorator as middleware for the HTTP handlers.

Notably, this does not log /pub /mpub (should it?).

This also adds 404, 405, and "panic" handlers so that those responses
are consistently formatted.

RFR @jehiah

@mreiferson mreiferson force-pushed the http_logs_610 branch 4 times, most recently from 963395f to 8a9f047 Compare July 26, 2015 15:18
@jehiah
Copy link
Member

jehiah commented Jul 28, 2015

LGTM; there are probably a dozen ways to approach this, but this one feels fine.

jehiah added a commit that referenced this pull request Jul 28, 2015
@jehiah jehiah merged commit df41bf9 into nsqio:master Jul 28, 2015
@mreiferson mreiferson deleted the http_logs_610 branch July 28, 2015 18:13
router.Handle("GET", "/node/:node", http_api.Decorate(s.nodeHandler, log))
router.Handle("GET", "/topic/:topic", http_api.Decorate(s.topicHandler, log))
router.Handle("GET", "/topic/:topic/:channel", http_api.Decorate(s.channelHandler, log))
router.Handle("GET", "/static/:asset", http_api.Decorate(s.embeddedAssetHandler, log))
Copy link
Member

Choose a reason for hiding this comment

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

@mreiferson turns out this doesn't work because log doesn't handle the response leaving nothing to render the response from the handler (ie: s.embededAssetHandler) into the response.

The effect is that requests for static assets return a 0 length content.

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