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.

Sign up for GitHub

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

Jump to bottom

Change monitoring hooks - and switch to middleware based architecture? #9

Open
lukaszkorecki opened this issue Nov 13, 2022 · 2 comments
Open

Change monitoring hooks - and switch to middleware based architecture? #9

lukaszkorecki opened this issue Nov 13, 2022 · 2 comments

Comments

@lukaszkorecki
Copy link
Contributor

lukaszkorecki commented Nov 13, 2022 •
edited
Loading

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
Copy link

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
Copy link

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@lukaszkorecki @lukaszkorecki-uz @madamova-ut