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

Add request/response logging middleware #322

Merged
merged 19 commits into from Nov 26, 2017
Merged

Add request/response logging middleware #322

merged 19 commits into from Nov 26, 2017

Conversation

bkeepers
Copy link
Contributor

@bkeepers bkeepers commented Nov 9, 2017

This PR is part of a series of changes to improve logging support in Probot: #320

This is an alternative to #270 and #314. It has the following advantages:

  • It sets X-Request-ID in the response, which is a value either provided by the client in the request or generated. This allows tracing of errors by request by searching for req.headers.x-request-id and the value of the request id.
  • Adds req.log which will include X-Request-ID in all log messages
  • Like Added request logging #270, it logs all request headers so they can be searched later
  • Like Add http-request-logging with morgan #314, it includes a nice format for normal response logging

TODO:

  • After Add context.log to add event context to each log message #321 merges, update this to use lib/logging.js and wrap the logger on req.log so it can be called as req.log(foo) or req.log.trace(foo)
  • Move custom req and res serializers into lib/serializers.js
  • Remove some of the noise when LOG_LEVEL=info
  • Round response time to 2 decimal places

@bkeepers
Copy link
Contributor Author

bkeepers commented Nov 9, 2017

Current log format in development:

20:00:55.030Z  INFO Probot: GET /ping 200 - 0.502784 ms (id=1fb3e537-90fb-4a29-ab00-d8f10faee694, duration=0.502784)
  HTTP/1.1 200 OK
  x-powered-by: Express
  x-request-id: 1fb3e537-90fb-4a29-ab00-d8f10faee694

@bkeepers bkeepers mentioned this pull request Nov 21, 2017
* origin/master:
  Fix two 404s to the API docs (#344)
  Add `context.log` to add event context to each log message (#321)
  fix(package): update github to version 12.0.3 (#340)
  chore(packaging): Remove semantic-release from CI (#341)
  refactor: switch ejs for hbs
  fix: Move unhandled rejection listener to cli scripts (#339)
  chore: Move index.js into lib/ (#338)
  Add option to develop Probot on Glitch (#306)
  refactor: absorb github-app
  chore: Setup Appveyor (#334)
  docs: Replace examples with include from website (#327)
…to request-logging

* 'request-logging' of https://github.com/probot/probot:
  Round response time to 2 digits
  Load serializers for tests
  Fix linting
  Include request in response log
  Move req/res serializers

# Conflicts:
#	lib/index.js
},

// Same as buyan's standard serializers, but gets headers as an object
// instead of a string.
Copy link
Member

Choose a reason for hiding this comment

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

Suuuuuper nitpicky, but there's an extra space here 😎

@@ -209,28 +209,3 @@ module.exports = (...args) => new Robot(...args)
* @typedef github
* @see {@link https://github.com/mikedeboer/node-github}
Copy link
Member

Choose a reason for hiding this comment

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

This is unrelated to your current work, but we should update this link.

This means all log messages tied to a request or webhook will have the same `id` attribute, allowing you to filter logs by the id.
@bkeepers bkeepers requested a review from a team November 25, 2017 21:25
@bkeepers
Copy link
Contributor Author

Ok, calling this good enough. I'd love a set of eyes. Here's the current default info output of an app starting up and receiving a webhook:

screen shot 2017-11-25 at 2 51 03 pm

Setting LOG_LEVEL=debug is more verbose and includes req/res headers and more details:

$ LOG_LEVEL=debug npm start

> probot-reminders@1.0.0 start /Users/bkeepers/projects/probot/reminders
> probot run ./index.js

21:27:45.921Z  INFO probot: GET /ping 200 - 2.87 ms (id=36cbef31-0b4b-4ebb-a6a2-059f6c89eb30)
21:27:45.923Z DEBUG probot:  (id=36cbef31-0b4b-4ebb-a6a2-059f6c89eb30, req.remoteAddress=::ffff:127.0.0.1, req.remotePort=49767, res.duration=2.87)
  GET /ping HTTP/1.1
  x-real-ip: 174.217.46.106
  x-forwarded-for: 174.217.46.106
  host: bkeepers.localtunnel.me
  x-forwarded-proto: https
  x-nginx-proxy: true
  connection: close
  --
  HTTP/1.1 200 OK
  x-powered-by: Express
  x-request-id: 36cbef31-0b4b-4ebb-a6a2-059f6c89eb30
Listening on https://bkeepers.localtunnel.me
21:27:46.188Z DEBUG probot: Scheduling interval (repository=bkeepers/test, delay=0, interval=900000)
21:28:06.758Z  INFO probot: Webhook received
  event: {
    "id": "b7007560-d21b-11e7-807b-96ed96b38a84",
    "event": "issue_comment.created",
    "repository": "robotland/test",
    "installation": 13055
  }
21:28:06.759Z  INFO probot: POST / 200 - 92.06 ms (id=b7007560-d21b-11e7-807b-96ed96b38a84)
21:28:06.760Z DEBUG probot:  (id=b7007560-d21b-11e7-807b-96ed96b38a84, req.remoteAddress=::ffff:127.0.0.1, req.remotePort=49766, res.duration=92.06)
  POST / HTTP/1.1
  x-real-ip: 192.30.252.42
  x-forwarded-for: 192.30.252.42
  host: bkeepers.localtunnel.me
  x-forwarded-proto: https
  x-nginx-proxy: true
  connection: close
  content-length: 9795
  accept: */*
  user-agent: GitHub-Hookshot/e20df6f
  x-github-event: issue_comment
  x-github-delivery: b7007560-d21b-11e7-807b-96ed96b38a84
  content-type: application/json
  x-hub-signature: sha1=3fc9ea44aec0b8dab4b9365f7d10fd4c3f79d6dc
  --
  HTTP/1.1 200 OK
  x-powered-by: Express
  x-request-id: b7007560-d21b-11e7-807b-96ed96b38a84
  content-type: application/json

@bkeepers
Copy link
Contributor Author

I'm not sure why the PR status is not updating, because it is passing.

@bkeepers bkeepers merged commit 1e25518 into master Nov 26, 2017
@bkeepers bkeepers deleted the request-logging branch November 26, 2017 04:16
This was referenced Nov 26, 2017
bkeepers added a commit that referenced this pull request Dec 6, 2017
* origin/master:
  Add example to README (#361)
  Enable Codecov (#360)
  Implement GHE Support (#331)
  refactor: Extract Sentry logging to a built-in plugin (#353)
  Log full requests as trace (#352)
  Improve GitHub logging (#351)
  General cleanup (#350)
  doc: Logging (#348)
  feat: Add request/response logging (#322)
  fix: Improve log messages for localtunnel (#349)

# Conflicts:
#	lib/index.js
#	lib/robot.js
#	test/robot.test.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants