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

allows for overriding the makeId function #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

collin
Copy link

@collin collin commented Jul 2, 2019

This allows for a customized request id generator.

In particular I've been using it with continuation-local-storage to get a "request id" that can be used by any async resource in a given express "request thread"

https://www.npmjs.com/package/continuation-local-storage

A bit like this:

const {
  createNamespace,
  getNamespace,
} = require('continuation-local-storage')
const volleyball = require('volleyball')
const makeId = require('./id')

const express = require('express')

const app = express()

const THREAD_ID_NS = 'THREAD_ID_NS'
const THREAD_ID = 'THREAD_ID'

const threadNS = createNamespace(THREAD_ID_NS)
app.use((req, res, next) => {
  const ns = getNamespace(THREAD_ID_NS)
  ns.bindEmitter(req)
  ns.bindEmitter(res)
  threadNS.run(() => next())
})

app.use(volleyball.custom({
  makeId: () => {
    const ns = getNamespace(THREAD_ID_NS)
    const threadId = makeId()
    ns.set(THREAD_ID, threadId)
    return threadId
  }
}))

function threadNSLogger (...args) {
  const ns = getNamespace(THREAD_ID_NS)
  const id = ns.get(THREAD_ID)
  if (id) {
    console.log(id, ...args)
  }
  else {
    console.log(...args)
  }
}


const Sequelize = require('sequelize')
const db = new Sequelize('postgres://localhost:5432/cls-logger-db', {
  logging: (query) => {
    if (process.env.LOG_SQL === "true") {
      threadNSLogger('——> ' + query)
    }
  }
})

}))

Assignee Tasks

  • added unit tests
  • written relevant docs

Copy link
Owner

@glebec glebec left a comment

Choose a reason for hiding this comment

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

I like this feature. Have some nits to pick; in rough order of priority:

  • possibly fill in empty test spec
  • remove unnecessary export property
  • prefer init-time branching
  • some small spelling typos
  • unnecessary formatting diffs

@@ -54,13 +54,17 @@ The `debug` property logs using the [`debug`](https://github.com/visionmedia/deb
| string | uses a new debug instance with a custom namespace |
| function | uses any function, such as a pre-generated debug instance. Note that the function will be called with colorized strings. |

The `makId` property specifies a custom request id generation function. It MUST be a funciton that returns a string.
Copy link
Owner

Choose a reason for hiding this comment

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

  • makId -> makeId
  • funciton -> function

* Log both upon **request** and **response**. Drawback: it is not necessarily clear which responses are for which requests.
- Log only upon **request**. Drawback: we cannot log the corresponding response, which happens later (if at all).
- Log only upon **response**, attaching the request. Drawback A: if the server never sends a response, e.g. due to a bug, the request will not be logged either. Drawback B: two temporally distinct events are conflated, misleadingly.
- Log both upon **request** and **response**. Drawback: it is not necessarily clear which responses are for which requests.
Copy link
Owner

Choose a reason for hiding this comment

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

Unnecessary formatting diff * -> - — not a blocker, but also if it can be undone that'd be swell.

- [morgan-debug](https://github.com/ChiperSoft/morgan-debug#readme)
- [winston](https://github.com/winstonjs/winston#readme)
- [express-winston](https://github.com/bithavoc/express-winston#readme)
- [node-bunyan](https://github.com/trentm/node-bunyan/#readme)
Copy link
Owner

Choose a reason for hiding this comment

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

More unnecessary * -> - formatting

@@ -9,6 +9,7 @@ const makeId = require('./id')
const sp = ' '

module.exports = Volleyball() // eslint-disable-line new-cap
module.exports.makeId = makeId
Copy link
Owner

Choose a reason for hiding this comment

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

Why are we adding this to the volleyball instance itself? As I understand it, we only need to make makeId a property of the config passed to volleyball.custom.

app.use(volleyball.custom({ makeId }))
})

it('logs reuests and responses', test)
Copy link
Owner

Choose a reason for hiding this comment

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

  • reuests -> requests
  • Empty spec; ideally we should capture output and see if the id matches expectations, I guess

@@ -18,7 +19,7 @@ function Volleyball(config = {}) {
// items shared between the request and response of just one cycle
const cycle = {
log: log,
id: makeId(),
id: config.makeId ? config.makeId() : makeId(),
Copy link
Owner

Choose a reason for hiding this comment

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

The compiler may be able to optimize this, but on principle I'd prefer init-time branching – add the following ca. line 17 (before defining function volleyball):

const idGenerator = config.makeId || makeId

Then cycle would be defined as:

const cycle = {
    log: log,
    id: idGenerator(),
    time: process.hrtime()
}

It may be overkill, but that's how I'd do it (similar to how I instantiate the log function above).

Copy link
Owner

Choose a reason for hiding this comment

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

Follow-up, I would also prefer to add a modicum of dynamic type-checking to this config.

if ((makeId in config) && (typeof config.makeId) !== 'function') {
    throw TypeError('Volleyball config option `makeId` must be a function')
}

(untested)

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.

2 participants