feat: expose requestRecorder on locals or server and as export

[tdeekens]

• Closes Expose in-built _request-observer_ #47

Summary

This pull request adds exposing the observeRequest function (from createRequestObserver) on the locals or server (depending if express or Hapi).

[codecov-io]

Codecov Report

Merging #48 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   99.22%   99.25%   +0.03%     
==========================================
  Files           8        8              
  Lines         129      135       +6     
  Branches       12       12              
==========================================
+ Hits          128      134       +6     
  Misses          1        1

Impacted Files	Coverage Δ
...create-request-recorder/create-request-recorder.js	100% <100%> (ø)
packages/express/modules/middleware/middleware.js	100% <100%> (ø)	⬆️
packages/hapi/modules/plugin/plugin.js	100% <100%> (ø)	⬆️

[dchambers]

This is amazing! Thanks so much for taking a look into this 🙏

To make this concrete, I can share my pre-existing getLabelValues definition:

getLabelValues: req => ({
    role: 'server',
    method: req.method,
    endpoint: endpointName(req),
  }),

The relevant bit here is the role: server part, which our specification says must be used for requests received by the server.

So that I can access the metrics in other parts of my codebase I have this code:

export const clientMetrics = {}

export default app => {
  const middleware = createMiddleware({ app, options })

  app.locals.Prometheus.register.setDefaultLabels(envLabels)

  clientMetrics.requestLatencySecondsObserver = app.locals.observeRequest

  clientMetrics.requestTimeoutsTotalCounter = new app.locals.Prometheus.Counter(
    {
      name: 'request_timeouts_total',
      help:
        'The number of timeouts or errors while connecting to external services',
      labelNames: labels,
    }
  )

  return middleware
}

Then, finally, I will have a utility like this in my fetch-utils.js file; used for all outbound network requests:

const createRequestObserver = () => {
  const start = process.hrtime()

  return res => {
    clientMetrics.requestLatencySecondsObserver(start, {
      labels: {
        // NOTE: I forgot to consider how I'd spread in the default set of labels here!
        role: 'client',
      },
    })

    return res
  }
}

which then allows me to write network request code like this:

return axios
  .get(url, updateOpts(options))
  .then(createRequestObserver())

The idea here is that we'd like a metric for both inbound and outbound requests, and in my company these metrics both have identical definitions except that inbound requests have a role of server and outbound requests have a role of client.

@tdeekens, do you have different ideas on how this might best be achieved now you've seen something more concrete?

[tdeekens]

Thanks, now I follow. Some thoughts:

• How you have to store/extract the observeRequest from the locals to make it available elsewhere is not ideal
  • I added a getRequestObserver we can just import const { getRequestObserver } = require('@promster/express'
  • It will additionally remain exposed on the locals of plugin

Additionally, looking at your example of

return axios
  .get(url, updateOpts(options))
  .then(createRequestObserver())

won't work as you have to take the start time before the Promise settles. I added an example to the readme in one of my last commits.

Lastly, your example of

return res => {
    clientMetrics.requestLatencySecondsObserver(start, {
      labels: {
        // NOTE: I forgot to consider how I'd spread in the default set of labels here!
        role: 'client',
      },
    })

is interesting but there is not much the library can help here as the request does not run through the express/Hapi you'd have to pass the default labels if you want them such as method, status_code and path.

Can you verify that the changes on this PR suit your use case? Then I'd merge and release this.

[tdeekens]

I renamed the request observer to request recorder. I found observer a bit misleading as it doesn't observe it just reports or records things. Observing would be more when it hooks into the Promise.

Everything remains the same just:

const { getRequestRecorder } = require('@promster/express');

changed.

[dchambers]

Hi @tdeekens, just been checking everything over on my end one last time, and I can confirm that this completely solves everything for me, plus cleans things up quite nicely what with the direct import and clearer naming; in the same spirit, I've even started directly importing Prometheus from @promster/metrics too as that further simplified things for me.

Just want to say a massive thank you for the personal time you contributed here 🥇. I feel slightly guilty taking so much of your time up, so if you accept donations I'd be very happy to contribute ❤️.

In case it helps others any, here's the code I've ended up with for a fully customized middleware plus some extra metrics in just a tiny bit of code (thanks promster 💯):

NOTE: Updated as per advice in comments below

import {
  createMiddleware,
  getRequestRecorder,
  Prometheus,
} from '@promster/express'

const labels = [
  'environment',
  'region',
  'version',
  'role',
  'method', // NOTE: built-in metric, but we override for upper-case
  'endpoint',
  'status_code', // NOTE: built-in metric
]

Prometheus.register.setDefaultLabels({
  environment: process.env.ENV_NAME || 'unknown',
  region: process.env.REGION_NAME || 'unknown',
  version: process.env.SERVICE_VERSION || 'unknown',
})

const endpointName = req =>
  req.originalUrl === '/graphql/api' && Array.isArray(req.body)
    ? req.body.map(op => op.operationName || 'unknown-operation-name').join('+')
    : req.originalUrl

const options = {
  metricNames: {
    httpRequestDurationInSeconds: 'request_latency_seconds',
    httpRequestsTotal: 'requests_inprogress_total',
  },
  buckets: [
    '0.005',
    '0.01',
    '0.025',
    '0.05',
    '0.075',
    '0.1',
    '0.25',
    '0.5',
    '0.75',
    '1.0',
    '2.5',
    '5.0',
    '7.5',
    '10.0',
  ],
  labels,
  getLabelValues: req => ({
    role: 'server',
    method: req.method,
    endpoint: endpointName(req),
  }),
}

export const createClientRequestRecorder = reqOptions => {
  const recordRequest = getRequestRecorder()
  const start = process.hrtime()

  return res => {
    recordRequest(start, {
      labels: {
        role: 'client',
        method: reqOptions.method.toUpperCase(),
        endpoint: reqOptions.url,
        status_code: res.status,
      },
    })
    return res
  }
}

export const requestTimeoutsTotalCounter = new Prometheus.Counter({
  name: 'request_timeouts_total',
  help: 'The number of timeouts while connecting to external services',
  labelNames: labels,
})

export default app => createMiddleware({ app, options })

[tdeekens]

Glad it helps. Don't feel guilty. I enjoy these kind of things and expect this also to help others maybe even me.

Regarding your code snippet: you should not import Prometheus from the metrics package. It's a transitive dependency of the express pkg. So you can just import it from there as it's exposed there too:

import { createMiddleware, getRequestRecorder, Prometheus } from '@promster/express'

I've honestly never received a donation during three years of Open Source with about ten libraries and would not expect anybody to donate. However, if you would like to buy me a coffee through that I would be thankful (PayPal: shopping(at)tdeekens.name. If not I am still thankful for the useful suggestion to the library and the really pleasant way of communicating with you.

[tdeekens]

Release in https://github.com/tdeekens/promster/releases/tag/%40promster%2Fexpress%402.1.0

[dchambers]

Release in https://github.com/tdeekens/promster/releases/tag/%40promster%2Fexpress%402.1.0

This worked perfectly. Thanks again 👍, and some coffees are on their way... ☕️