Change monitoring hooks - and switch to middleware based architecture?

[lukaszkorecki]

Two things:

Pass request metrics as data to the monitoring hooks

Rather than generating Graphite-style metrics, like here:

nomnom.duckula/src/duckula/handler.clj

Lines 40 to 60 in 2e0a2fa

	(defn build-metric-keys
	"For each endpoint it constructs a list of metric keys
	to be used when tracking timing, rates and errors
	Example for name test-api and /some/endpoint it will create
	test-api.some.endpoint
	test-api.some.endpoint.success
	test-api.some.endpoint.error
	test-api.some.endpoint.failure"
	[{:keys [endpoints prefix] :as config}]
	(->> endpoints
	(map (fn metric-key-builder
	[[path _conf]]
	(let [metric-key (str (:name config) (s/replace (str prefix path) \/ \.))
	success-key (str metric-key ".success")
	error-key (str metric-key ".error")
	failure-key (str metric-key ".failure")]
	(hash-map (str prefix path) [metric-key
	success-key
	error-key
	failure-key]))))
	(into {})))

Monitoring component should receive something like this:

(monitoring/on-success monitoring {:endpoint endpoint :status status :response response-map})
(monitoring/on-not-found monitoring {:endpoint endpoint :status status})
(monitoring/on-error monitoring  {:endpoint endpoint :error error })

This way it's down to the actual monitoring component implementation to turn these signals into metrics in whatever format it needs.

Move off monitoring component

Perhaps this would also make it easier to work with Duckula in general - and make it not depend on Component at all. Handler builder would work out details of:

• request timing
• whether the internal status was :duckula.response/success or :duckula.response/error
• was the endpoint found
• type of valdiation error

All of that would be stored in :duckula/context key on the response map.

Then a new middleware would read that and report metrics, generate logs, remove :duckula/context map and pass it down to other response middlewares down the chain.

Much simpler architecture and something that we were discussing in the context of nomnom-insights/nomnom.bunnicula#3

[madamova-ut]

I agree not making the things dependent on each other is probably better. But it would mean refactoring each of the services which uses duckula right (with mono repo of course easier).

[lukaszkorecki-uz]

Yeah, I was thinking about this too from refactoring perspective. I think this can be done in stages without changing how Duckula works:

• add the request context map to the Ring response (it would be transparent to the current monitoring component implementations, since they receive method calls, not the whole ring data)
• provide a no-op implementation of the monitoring so it can be an opt-out
• add the new monitoring middleware to Duckula that relies on :duckula.handler/context (name TBC)
• when refactoring the change will boil down to:
  • swapping current monitoring component with duckula.monitoring.noop
  • activating the new monitoring middleware, kinda like this:

    nomnom.duckula/src/duckula/middleware.clj

    Line 19 in 34107b2

    (defn with-monitoring