Skip to content
This repository was archived by the owner on Feb 18, 2021. It is now read-only.

Conversation

tomuber
Copy link

@tomuber tomuber commented Aug 29, 2014

cc @Raynos @kumikoda @mlmorg

Updated based on Jake's feedback:
-Don't provide a middleware
-I opted to provide a sane default for when the user does not provide a statsdKey

@Raynos
Copy link
Contributor

Raynos commented Aug 29, 2014

If we are going to pave a cowpath we should probably do a little better then this.

We can change the express-statsd module to be

var statsd = require('express-statsd');
var computeKey = require('express-statsd/compute-key');

statsd(req, res, {
  statsdKey: computeKey(req, res)
}, cb)

We can put the code you have for computing the key value in a compute-key function.

We can then get rid of the middleware stuff.

@kumikoda
Copy link

+1 on what jake said. No middleware stuff!

@tomuber tomuber force-pushed the middleware branch 4 times, most recently from 4030092 to b5ff038 Compare August 30, 2014 08:49
@tomuber tomuber changed the title Express-signature middleware (req, res, next) that logs timing for http ... Add sane default behavior for missing statsdKey Aug 30, 2014
@tomuber
Copy link
Author

tomuber commented Aug 30, 2014

@Raynos @mlmorg @kumikoda I updated this based on jake's feedback, I opted to provide a sane default for when the user doesn't provide a statsdKey

the top level URI in addition to the method
@tomuber
Copy link
Author

tomuber commented Aug 30, 2014

This is failing on lynx not being available for node 0.8

@mlmorg
Copy link
Contributor

mlmorg commented Aug 30, 2014

I think this should be two libraries: http-statsd and express-statsd. http-statsd can follow @Raynos' interface while express-statsd can build upon it and allow you to do something like:

server.get('/', 'root.statsd-key', function (req, res) {...});

Also, let me mention my dislike for trying to provide a "sane" default for three reasons:

  1. With this approach, you'd now be logging any pathname + query param that a user decides to hit, regardless of whether it's an actual route that the service supports or not, e.g. /mistyped-route?foo=bar
  2. You'd log all permutations of routes like /users/:id separately.
  3. You may eventually run into collisions, e.g. /foo/bar and /foo_bar

We may be able to provide some ok defaults based on express' req.route interface but, again, it will never be perfect (e.g. * wildcards and collisions like issue 3 above) and that's why I would rather the eng just have to set it themselves.

@Raynos
Copy link
Contributor

Raynos commented Aug 30, 2014

@mlmorg you are correct.

What you really want is integration with a routing table -.-

statsd(req, res, {
  routePattern: x
})

Which should work fine.

@mlmorg

  • wildcards and collisions like issue 3 above

can we not just find a way of encoding a route pattern without running into collisions?

@tomuber
Copy link
Author

tomuber commented Sep 2, 2014

The default is only for the top level URI, so # 2/3 won't apply. Re: # 1 it'll log the pathname and not the query params.

@mlmorg
Copy link
Contributor

mlmorg commented Sep 10, 2014

What's the point of only logging the content within the first /'s?

@CLAassistant
Copy link

CLAassistant commented Jun 27, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants