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

Enable calculating id based on http request #47

Closed
wants to merge 11 commits into from

Conversation

tan-tan-kanarek
Copy link

@tan-tan-kanarek tan-tan-kanarek commented Nov 26, 2020

Refs: #25

@puzpuzpuz
Copy link
Owner

@tan-tan-kanarek thanks for your contribution. Could you explain the use case behind this addition?

@tan-tan-kanarek
Copy link
Author

tan-tan-kanarek commented Nov 26, 2020

I will share my use case, but I'm sure there are many other similar use cases.

var app = express();
// auth is adding session object to the req object.
app.use(auth, rTracer.expressMiddleware({
    requestIdFactory: req => {
        let userSid = req.session ? req.session.userSid : null;
        let requestId = req.headers['x-requestid'];
        let sequenceSid = req.headers['x-sequencesid'];

        if(!requestId) {
            requestId = Math.random().toString(36).substring(2, 12);
        }
        return {sequenceSid, requestId, userSid}; // I return object but it could be a calculated string as well
    }
}));```

Later in my logger transformer I use the user info, the sequence (session) info and request-id.
Instead of direct headers I could take the session-id from the request cookies or some other data from the query-string.

@puzpuzpuz puzpuzpuz added this to the 2.6.0 milestone Nov 26, 2020
@puzpuzpuz puzpuzpuz added the enhancement New feature or request label Nov 26, 2020
@puzpuzpuz
Copy link
Owner

I see. Let me review the changes.

Copy link
Owner

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

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

Could you also add tests to verify this change?

index.d.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@puzpuzpuz
Copy link
Owner

Do you need any advice on tests? I see some changes, but they're not adding tests.

@tan-tan-kanarek
Copy link
Author

Thank you, advice on tests will be appreciated.

@puzpuzpuz
Copy link
Owner

It could be something like the following test:

  test('calls request id factory with req', () => {
    const app = express()
    const idFactory = (req) => req

    app.use(rTracer.expressMiddleware({
      requestIdFactory: idFactory
    }))

    app.get('/test', (req, res) => {
      if (req === rTracer.id()) {
        res.end()
      } else {
        res.status(500).end()
      }
    })

    return request(app).get('/test')
      .then(res => {
        expect(res.statusCode).toBe(200)
      })
  })

One more thing to notice here is that the suggested implementation always passed request object into requestIdFactory. The default value of that option is uuidv1 and such behavior may lead to various issues with collisions between req and options expected by uuidv1. So, it's better to pass the object only when the requestIdFactory option is set by the user.

src/rtracer.js Outdated Show resolved Hide resolved
@@ -96,7 +96,7 @@ const fastifyPlugin = (fastify, options, next) => {
if (useFastifyRequestId) {
requestId = requestId || request.id
}
requestId = requestId || requestIdFactory()
requestId = requestId || (requestIdFactory ? requestIdFactory(request.raw) : uuidv1())
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
requestId = requestId || (requestIdFactory ? requestIdFactory(request.raw) : uuidv1())
requestId = requestId || (requestIdFactory ? requestIdFactory(request) : uuidv1())

Copy link
Author

Choose a reason for hiding this comment

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

The raw contains all the info one might need from the request and will be compatible between all versions of fastify.

Copy link
Owner

Choose a reason for hiding this comment

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

Other non-Express middlewares and plugins from this PR provide framework specific request objects, so having request.raw only in Fastify middleware doesn't seem to be consistent. Also, frameworks usually expose convenient APIs in their request wrapper objects, so to me it makes more sense to allow user to deal with the familiar API (which is framework's request wrapper).

tests/fastifyv2.test.js Outdated Show resolved Hide resolved
tests/fastifyv2.test.js Outdated Show resolved Hide resolved
tests/koa.test.js Outdated Show resolved Hide resolved
tests/koav1.test.js Outdated Show resolved Hide resolved
tests/koav1.test.js Outdated Show resolved Hide resolved
@puzpuzpuz
Copy link
Owner

Closing this one in favor of #48

@puzpuzpuz puzpuzpuz closed this Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants