Nit: The name queued_seconds implies an integer, consider changing the type or the name.

I don't think it necessarily implies integer; time.Duration's Seconds() returns a float64.

Fair enough 😄.

In the DB, usually it means seconds:

    lifetime_seconds bigint DEFAULT 86400 NOT NULL,
    timeout_seconds integer NOT NULL
    connection_timeout_seconds integer DEFAULT 0 NOT NULL,

But fine to keep as float if we care about the precision (although I'd prefer to multiply the number to required precision in int).

I suppose I don't strictly need to keep sub-second precision on the length of time messages were queued for.
I'll switch to integer 👍

This is backing a Prometheus metric, which are always floats, and by convention end in the SI unit, which for time is seconds. It implies no precision.

This should be a float: there just isn't any compelling reason to pre-judge the required precision. This is not a banking application, nor do we need to worry about the relative speed of integer vs floating point arithmetic.

The name queued_seconds is appropriate.

maybe "notification_template_id" --- there is another, more user-facing concept of "template" which people might assume this refers to.

DispatchCount, TempFailureCount, and PermFailureCount might make sense as a single metric with different "disposition" (or "result") labels. The help text refers to them as "notifications", but I think that's misleading. What we are counting is "notification attempts" or "dispatch attempts", which might individually end up in success or some kind of failure. In particular, in the case of failure and retry, the number of delivery attempts can exceed the number of notifications, so it's important to make the distinction.

Prometheus guidelines say that something should be a single metric with labels (vs multiple metrics) when the sum or average of the label is sensible and coherent. Filtering by method and summing across disposition would be useful, for example, to track the number of web requests we are making via Webhook delivery.

Super idea! Simplifies things a lot.

A really useful Gauge is the number of "in progress" delivery attempts. You increment it when you kick off the delivery goroutine and decrement it when you send to success or failure channels. Can help spot hung handlers/leaked goroutines

Good idea!

accumulating counts should end in the units and total as in coderd_notifications_successful_deliveries_total, where "deliveries" is the unit (could use "dispatch_attempts", or something similar).

https://prometheus.io/docs/practices/naming/

Wow, 4 months out of Grafana Labs and I forget all my Prometheus manners.
Thanks!

Sounds too generic to me. "The number of delivery attempt results waiting to be flushed to the store" ?

Do we need to store this on the row? It feels a little strange and ambiguous. If the notification_message is currently queued, then it's a bit meaningless, since we don't know for how much longer it will be queued. I.e. it is at present being returned on Enqueue, but is meaningless in that context.

When we dequeue it, can't the notifier compute how long it was on the queue based on it's created_at or updated_at time? Or, if we really want to ensure all the measurements are based on the central PostgreSQL server clock, then it can part of the result returned to the notifier without having to write it back to the database.

Yeah I agree it's a bit awkward.

The problem is that updated_at is modified when the message is enqueued.

-- name: AcquireNotificationMessages :many
WITH acquired AS (
    UPDATE
        notification_messages
            SET ...
                updated_at = NOW(),

I tried various approaches to storing the updated_at value prior to its being modified here, but this approach seemed simpler.

"queued" should refer to the time between attempts; I can't use created_at for that reason. Good call on its being returned via EnqueueNotificationMessage; that's definitely not meaningful.

I should probably set it to 0 again if the message is marked as failed; that way the value will always be fairly meaningful:

• pending: value should be 0
• leased/sent: value should be present
• *failure/: value should be 0

I think it would be cleaner to just compute this metric on-Collect, rather than Seting it when something reads or writes to these channels. As an illustration of how easy it is to miss a spot, note that you have forgotten to set it in process() after a failed call to prepare().

I agree although changing to a Collect-based approach isn't worth the complexity IMHO.
This metric is updated in syncUpdates every 2s (by default), so that's probably good enough™️ for now.

I was initially planning to give syncUpdates the responsibility of updating this exclusively, but since StoreSyncInterval is tuneable, operators could mess with the metric collection in a way that is difficult to reason about.

I'll add the missing update, thanks.

We say this should be less than the fetch interval, which is 15s --- so I think there should be more buckets <15s. In fact, it should be more like 15/(n+1) where n is the number of Coderd replicas, assuming that notifications usually get picked up by the next fetch. Seeing the average move up toward 15s is way more important than the distinction between 10minutes, 1 hour and 1 day, all of which indicate that notifications are just blocked entirely.

Fair point.

should be 0.001, 0.005, 0.01, 0.05 on the bottom end, right?

I don't see much value in distinguishing between 1ms, 5ms, and 10ms; for most systems the latency will be higher than that (I'm assuming). The high end is what we're more concerned about here since that's indicative of a problem. In other words: this metric is more for diagnosing issues than measuring performance of the endpoint/server.

Dispatching, say, a webhook, within the same cluster could be a few milliseconds latency. Same geographic region could be 10-20ms.

Almost nothing is going to be 1ms unless it's extremely simple and extremely close, but in 50ms you could be dispatching across an entire continent. With buckets like this you could have something that normally returns in 15ms take 3x its normal time and be close to falling over and this metric would not clock it. That seems much more important than 30s, 60s, 120s.

I hear you and I can see where you're coming from.

Let's wait for users to ask for this. I think there's sufficient coverage for now, and the additional datapoints are likely not going to be very useful for the majority of operators IMHO.

Also a count of success/failure updates flushed to the database.

Addresses #13537 (comment)