github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-09-24 04:46:14 +00:00
Code Issues Packages Projects Releases Wiki Activity
penpot/backend/test/backend_tests/storage_metrics_test.clj
Andrey Antukh 452f38cf5d
✨ Add Prometheus metrics for storage operations (#11700) 
* ✨ Add storage operation metrics for S3 and buckets

Expose Prometheus metrics for the object storage subsystem.

The S3 backend now attaches an AWS SDK MetricPublisher that counts
API calls, retries and latency per operation and target. The storage
layer counts logical operations and deduplication outcomes per Penpot
bucket, and the assets handlers count served requests per route.

Closes #11676

AI-assisted-by: muse-spark-1.3-contributor

* ✨ Fix storage metrics labels, errors and test gaps

Address the review findings on the storage metrics commit.

Label reads with the object's own backend, count failed asset
serving as errors without swallowing them, and cover the failed
S3 call, S3 asset path and permission-denied branches with tests.
Also share the label helper and reuse the metrics test helper.

Closes #11676

AI-assisted-by: muse-spark-1.3-contributor

* ✨ Harden storage metrics and fill test gaps

Address the second-round review findings on storage metrics.

Unknown backends now fail explicitly and count as errors, exists
stays paired with its dedup outcome, and the thumbnail, missing
storage, expired reads, unknown buckets and write failure paths
are covered by tests. Label coercion goes through the shared
metrics helper.

Closes #11676

AI-assisted-by: muse-spark-1.3-contributor

* ✨ Harden storage metrics accuracy and coverage

Address the full-branch review findings on storage metrics.

Touch and delete emit only on changed rows, reads emit after the
backend fetch, unknown backends fail explicitly, and tempfile
mismatches count as unauthorized. Publisher nil policy, pairing
rules and attempt semantics are documented and covered by tests.

Closes #11676

AI-assisted-by: muse-spark-1.3-contributor

* ✨ Address full-branch review findings on storage metrics

Touch and delete resolve labels from the row, reads stay paired,
failures are covered by tests, and logging, ranges and docs are
tightened. Includes the label helper unit tests and the retries
wording clarification.

Closes #11676

AI-assisted-by: muse-spark-1.3-contributor

* ⚡ Label touch and del metrics from UPDATE RETURNING

The storage metrics change resolved metric labels for touch-object!
and del-object! with an extra SELECT per id-based call. Since app.main
instruments storage unconditionally, every GC collector and binfile
import paid that extra round trip: deleting a team with 10k media
objects doubled the storage_object statements exactly on the paths
that already process the most rows.

touch-object! and del-object! now take only the object id (UUID) and
read the labels from the updated row itself via RETURNING id, backend,
metadata: one statement, no pre-read, and labels that always match the
row actually mutated. del-object! additionally guards on deleted_at
IS NULL, so a repeated delete returns false and emits no metric.

Also from the review of the full branch: extract the duplicated
serve/emit/rethrow block in app.http.assets into one helper; give
penpot_storage_s3_timing explicit histogram buckets up to 60s (the
default cap at 7.5s hid the slow S3 calls the metric exists for);
drop the unused ::target-id config key from the S3 backend and
hardcode the :default target label until per-bucket routing lands.

AI-assisted-by: glm-5.3-flash

* ✨ Harden storage metric recording and definitions

The metric definition schema is now closed and declares every key
the collectors read: buckets, quantiles, max-age and reg. A typo
such as a misspelled ::mdef/buckets used to compile and silently
fall back to the default histogram buckets; it now fails at
startup.

The asset result-label fallback coerced an absent status to 500,
so a future serve path without a status would have counted
successes as errors. The mapping is now explicit and documented:
served below 400, unauthorized for 401/403, not-found for 404,
and error for everything else, including an absent status.

The never-fail try/catch around metric recording existed four
times with drift. One app.metrics/run-safe! helper replaces them:
it no-ops on a nil metrics instance and logs the first failure
per hint at warn level, then at debug, so a broken setup surfaces
once without flooding the log. The S3 publisher keeps its outer
try/catch: it is the SDK MetricPublisher contract boundary.

AI-assisted-by: glm-5.3-flash

* ✨ Make metrics mandatory and run! safe by default

Recording a metric must never change the behavior of the operation
being measured, so `run!` now catches recording failures itself: the
first failure per metric id logs at warn, later ones at debug. This
replaces the `run-safe!` helper, whose four copies had drifted, and
applies the guarantee to every emit site instead of only storage.

The metrics instance precondition is a plain assert, and the collector
lookup stays outside the recording guard, so a missing instance fails
hard even when asserts are disabled. Metrics is therefore no longer
optional: the storage, s3-backend and db-pool schemas require
`::mtx/metrics`, and the assets handler cfg always carries it.

`wrap-publisher` no longer returns nil for a nil instance, and the db
pool wires the prometheus tracker unconditionally.

AI-assisted-by: deepseek-v4.1-flash
2026-09-23 15:25:21 +02:00

308 lines
15 KiB
Clojure
Raw Blame History

;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS SUBSIDIARY SL
(ns backend-tests.storage-metrics-test
(:require
[app.common.time :as ct]
[app.common.uuid :as uuid]
[app.main :as main]
[app.metrics :as mtx]
[app.metrics.definition :as-alias mdef]
[app.storage :as sto]
[backend-tests.helpers :as th]
[clojure.test :as t]
[datoteka.fs :as fs]
[integrant.core :as ig]
[mockery.core :refer [with-mocks]])
(:import
io.prometheus.client.Counter
io.prometheus.client.Counter$Child))
(t/use-fixtures :once th/state-init)
(t/use-fixtures :each (th/serial
th/database-reset
th/clean-storage))
(defn- make-metrics
[]
(ig/init-key :app.metrics/metrics
{:default (select-keys main/default-metrics
[:storage-operations
:storage-dedup])}))
(defn- configure-storage-backend
[storage]
(assoc storage ::sto/backend :fs))
(defn- with-metrics
[storage metrics]
(assoc storage ::mtx/metrics metrics))
(defn- counter-value
[metrics id labels]
(let [collector (mtx/get-collector metrics id)
instance (::mdef/instance collector)
child (.labels ^Counter instance (into-array String labels))]
(.get ^Counter$Child child)))
(defn- put!
[storage content bucket hash]
(sto/put-object! storage (cond-> {::sto/content (sto/content content)
:bucket bucket
:content-type "text/plain"}
(some? hash)
(assoc ::sto/deduplicate? true
::sto/content (sto/wrap-with-hash
(sto/content content)
hash)))))
(t/deftest put-emits-op-and-dedup-miss
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))]
(put! storage "content" "file-media-object" "hash-miss")
(t/is (= 1.0 (counter-value metrics :storage-operations ["put" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-dedup ["miss" "file-media-object"])))))
(t/deftest dedup-hit-reuses-object-without-put
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object1 (put! storage "content" "file-media-object" "hash-hit")
object2 (put! storage "content" "file-media-object" "hash-hit")]
(t/is (= (:id object1) (:id object2)))
(t/is (= 1.0 (counter-value metrics :storage-operations ["put" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-dedup ["miss" "file-media-object"])))
(t/is (= 1.0 (counter-value metrics :storage-dedup ["hit" "file-media-object"])))
(t/is (= 1.0 (counter-value metrics :storage-operations ["exists" "file-media-object" "fs"])))))
(t/deftest tempfile-skips-dedup
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object1 (put! storage "content" "tempfile" "hash-temp")
object2 (put! storage "content" "tempfile" "hash-temp")]
(t/is (not= (:id object1) (:id object2)))
(t/is (= 2.0 (counter-value metrics :storage-operations ["put" "tempfile" "fs"])))
(t/is (= 2.0 (counter-value metrics :storage-dedup ["skip" "tempfile"])))))
(t/deftest repair-rewrites-missing-blob
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object1 (put! storage "content" "file-media-object" "hash-repair")]
(fs/delete (sto/get-object-path storage object1))
(let [object2 (put! storage "content" "file-media-object" "hash-repair")]
(t/is (= (:id object1) (:id object2)))
(t/is (= "content" (slurp (sto/get-object-data storage object2))))
(t/is (= 1.0 (counter-value metrics :storage-operations ["put" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-operations ["repair" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-dedup ["miss" "file-media-object"])))
(t/is (= 1.0 (counter-value metrics :storage-dedup ["repair" "file-media-object"]))))))
(t/deftest get-touch-and-del-emit-ops
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (put! storage "content" "file-media-object" nil)]
(t/is (= "content" (slurp (sto/get-object-data storage object))))
(t/is (bytes? (sto/get-object-bytes storage object)))
(t/is (true? (sto/touch-object! storage (:id object))))
(t/is (true? (sto/del-object! storage (:id object))))
(t/is (= 1.0 (counter-value metrics :storage-operations ["get-data" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-operations ["get-bytes" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-operations ["touch" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-operations ["del" "file-media-object" "fs"])))))
(t/deftest storage-requires-metrics
;; Metrics is no longer optional: a storage map without it does not
;; validate, and any operation that records fails loudly instead of
;; silently dropping the measurement. `Throwable` covers both the
;; schema assert (elided unless :backend-asserts is on) and the
;; `run!` error.
(let [storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(dissoc ::mtx/metrics))]
(t/is (false? (sto/valid-storage? storage)))
(t/is (thrown? Throwable
(sto/put-object! storage {::sto/content (sto/content "content")
:bucket "file-media-object"
:content-type "text/plain"})))))
(t/deftest default-metrics-definitions
(let [defs main/default-metrics]
(t/is (= "penpot_storage_operations_total" (::mdef/name (:storage-operations defs))))
(t/is (= ["op" "bucket" "backend"] (::mdef/labels (:storage-operations defs))))
(t/is (= "penpot_storage_dedup_total" (::mdef/name (:storage-dedup defs))))
(t/is (= ["result" "bucket"] (::mdef/labels (:storage-dedup defs))))))
(t/deftest read-labels-object-backend
;; An object keeps its own backend; reads must be labeled with it even
;; when the storage default points elsewhere (e.g. after a migration).
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (put! storage "content" "file-media-object" nil)
storage (assoc storage ::sto/backend :s3)]
(t/is (= "content" (slurp (sto/get-object-data storage object))))
(t/is (= 1.0 (counter-value metrics :storage-operations ["get-data" "file-media-object" "fs"])))
(t/is (= 0.0 (counter-value metrics :storage-operations ["get-data" "file-media-object" "s3"])))))
(t/deftest touch-and-del-by-id-label-row-bucket
;; Production callers pass UUIDs, not objects: the labels come from
;; the updated row itself (RETURNING), no extra SELECT.
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (put! storage "content" "file-media-object" nil)
id (:id object)]
(t/is (true? (sto/touch-object! storage id)))
(t/is (true? (sto/del-object! storage id)))
(t/is (= 1.0 (counter-value metrics :storage-operations ["touch" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-operations ["del" "file-media-object" "fs"])))))
(t/deftest repeated-del-is-idempotent-noop
;; A second del of the same id does not match (deleted_at IS NULL
;; guard): returns false, changes nothing and emits no metric.
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (put! storage "content" "file-media-object" nil)
id (:id object)]
(t/is (true? (sto/del-object! storage id)))
(t/is (false? (sto/del-object! storage id)))
(t/is (= 1.0 (counter-value metrics :storage-operations ["del" "file-media-object" "fs"])))))
(t/deftest touch-and-del-missing-id-emits-nothing
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
id (uuid/next)]
(t/is (false? (sto/touch-object! storage id)))
(t/is (false? (sto/del-object! storage id)))
(t/is (= 0.0 (counter-value metrics :storage-operations ["touch" "unknown" "fs"])))
(t/is (= 0.0 (counter-value metrics :storage-operations ["del" "unknown" "fs"])))))
(t/deftest touch-and-del-emit-once
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (put! storage "content" "file-media-object" nil)]
(t/is (true? (sto/touch-object! storage (:id object))))
(t/is (true? (sto/del-object! storage (:id object))))
(t/is (= 1.0 (counter-value metrics :storage-operations ["touch" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-operations ["del" "file-media-object" "fs"])))))
(t/deftest expired-object-emits-nothing
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (sto/put-object! storage {::sto/content (sto/content "content")
::sto/expired-at (ct/minus (ct/now) (ct/duration {:hours 1}))
:bucket "file-media-object"
:content-type "text/plain"})]
(t/is (nil? (sto/get-object-data storage object)))
(t/is (= 0.0 (counter-value metrics :storage-operations ["get-data" "file-media-object" "fs"])))))
(t/deftest failed-probe-emits-nothing
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))]
(put! storage "content" "file-media-object" "hash-probe-fail")
(with-mocks [_mock {:target 'app.storage.impl/exists-object?
:throw (ex-info "boom" {})}]
(t/is (thrown? clojure.lang.ExceptionInfo
(put! storage "content" "file-media-object" "hash-probe-fail"))))
(t/is (= 1.0 (counter-value metrics :storage-operations ["put" "file-media-object" "fs"])))
(t/is (= 0.0 (counter-value metrics :storage-operations ["exists" "file-media-object" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-dedup ["miss" "file-media-object"])))
(t/is (= 0.0 (counter-value metrics :storage-dedup ["hit" "file-media-object"])))
(t/is (= 0.0 (counter-value metrics :storage-dedup ["repair" "file-media-object"])))))
(t/deftest expired-object-bytes-emits-nothing
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (sto/put-object! storage {::sto/content (sto/content "content")
::sto/expired-at (ct/minus (ct/now) (ct/duration {:hours 1}))
:bucket "file-media-object"
:content-type "text/plain"})]
(t/is (nil? (sto/get-object-bytes storage object)))
(t/is (= 0.0 (counter-value metrics :storage-operations ["get-bytes" "file-media-object" "fs"])))))
(t/deftest put-without-bucket-labels-unknown
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (sto/put-object! storage {::sto/content (sto/content "content")
:content-type "text/plain"})]
(t/is (sto/object? object))
(t/is (= 1.0 (counter-value metrics :storage-operations ["put" "unknown" "fs"])))
(t/is (= 1.0 (counter-value metrics :storage-dedup ["skip" "unknown"])))))
(t/deftest failed-write-emits-nothing
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))]
(with-mocks [_mock {:target 'app.storage.impl/put-object
:throw (ex-info "boom" {})}]
(t/is (thrown? clojure.lang.ExceptionInfo
(put! storage "content" "file-media-object" "hash-write-fail"))))
(t/is (= 0.0 (counter-value metrics :storage-operations ["put" "file-media-object" "fs"])))
(t/is (= 0.0 (counter-value metrics :storage-dedup ["miss" "file-media-object"])))))
(t/deftest put-survives-metrics-failure
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))]
(with-mocks [_mock {:target 'app.metrics/run-collector!
:throw (ex-info "boom" {})}]
(let [object (put! storage "content" "file-media-object" "hash-metrics-fail")]
(t/is (sto/object? object))
(t/is (= "content" (slurp (sto/get-object-data storage object))))))))
(t/deftest failed-read-emits-nothing
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (put! storage "content" "file-media-object" nil)]
(with-mocks [_mock {:target 'app.storage.impl/get-object-data
:throw (ex-info "boom" {})}]
(t/is (thrown? clojure.lang.ExceptionInfo
(sto/get-object-data storage object))))
(t/is (= 1.0 (counter-value metrics :storage-operations ["put" "file-media-object" "fs"])))
(t/is (= 0.0 (counter-value metrics :storage-operations ["get-data" "file-media-object" "fs"])))))
(t/deftest failed-bytes-read-emits-nothing
(let [metrics (make-metrics)
storage (-> (:app.storage/storage th/*system*)
(configure-storage-backend)
(with-metrics metrics))
object (put! storage "content" "file-media-object" nil)]
(with-mocks [_mock {:target 'app.storage.impl/get-object-bytes
:throw (ex-info "boom" {})}]
(t/is (thrown? clojure.lang.ExceptionInfo
(sto/get-object-bytes storage object))))
(t/is (= 1.0 (counter-value metrics :storage-operations ["put" "file-media-object" "fs"])))
(t/is (= 0.0 (counter-value metrics :storage-operations ["get-bytes" "file-media-object" "fs"])))))
Reference in New Issue View Git Blame Copy Permalink