diff --git a/.serena/memories/backend/storage.md b/.serena/memories/backend/storage.md index 6ab862a311..43c925eeec 100644 --- a/.serena/memories/backend/storage.md +++ b/.serena/memories/backend/storage.md @@ -24,7 +24,7 @@ - `get-object` excludes rows with `deleted_at`. - Existing object values can remain readable until physical deletion. - `:expired-at` blocks reads after the expiration time. -- `del-object!` sets `deleted_at`. It does not remove backend content. +- `del-object!` sets `deleted_at` on live rows only (`deleted_at IS NULL`): a repeated call returns `false`. It does not remove backend content. - `storage-gc-deleted` removes the database row and backend content after the deletion delay. - `storage-gc-touched` finds references before it sets `deleted_at`. - `objects-gc` removes deleted domain rows and touches their storage object IDs. @@ -118,3 +118,20 @@ Since `put-object!` uses backend-specific operations (`impl/resolve-backend` + ` - The `file_data.metadata.storage-ref-id` value points to the storage object. - `fdata/upsert!` touches a storage object from incoming metadata before it stores the new row. - File snapshots use `file_data` for snapshot data and `file_change` for snapshot metadata. + +## Metrics + +- `bucket` is always the Penpot logical bucket (object metadata), never an S3 bucket. Unknown/absent buckets are labeled `"unknown"`. +- `target` is the physical S3 destination id. Today it is always `"default"` (hardcoded in `app.storage.s3/build-s3-client`; the `::target-id` config key was removed as unused until the per-bucket routing plan lands). +- Physical S3 API calls (AWS SDK `MetricPublisher`, `app.storage.s3.metrics`): + - `penpot_storage_s3_requests_total{operation,target,result}` — one count per logical SDK call (the published `ApiCall` collection, not per attempt); retries are counted apart in `retries_total`, so total attempts = `requests + retries`. `result` is `"ok"` only when the SDK reports success as exactly `true`; a missing success flag counts as `error`. + - `penpot_storage_s3_retries_total{operation,target}` — SDK retry count. + - `penpot_storage_s3_timing{operation,target}` — call latency histogram (ms); explicit buckets up to 60000 ms (S3 slow calls exceed the default 7500 ms cap). +- Logical storage operations (`app.storage`, `::mtx/metrics` required by the schema): + - `penpot_storage_operations_total{op,bucket,backend}` — `put`, `repair`, `get-data`, `get-bytes`, `del`, `touch`, `exists`. All ops are success-only: `put`/`repair` emit after the backend write, `get-*` after the backend fetch opens, `touch`/`del` only when a row actually changed. `touch-object!`/`del-object!` take the object id (UUID) only — no object overload. Labels come from the updated row itself via `UPDATE ... RETURNING id, backend, metadata` (no extra `SELECT`); with no row matched they emit nothing. `del-object!` only matches live rows (`deleted_at IS NULL`): a repeated del returns `false` and emits nothing. Post-open stream read errors stay counted as attempts. `del` only marks `deleted_at`; physical deletion is a GC concern. `exists` is emitted per deduplication-hit probe, always paired with a `hit`/`repair` outcome (never on probe failure), not per user-facing existence check. + - `penpot_storage_dedup_total{result,bucket}` — `hit`, `miss`, `repair`, `skip`. +- Asset serving (`app.http.assets`, `::mtx/metrics` required in the handler cfg): + - `penpot_storage_asset_requests_total{route,backend,bucket,result}` — `route` is `by-id`, `by-file-media-id`, or `thumbnail`; `result` is `served` (<400), `not-found` (404), `unauthorized` (401/403), or `error` (everything else, including a nil/non-number status: every serve path must set `::yres/status`). Serve-path exceptions are counted by `serve-object-measured` and then rethrown. Permission-denied file-media requests and tempfile ownership mismatches both answer HTTP 404 (to avoid leaking existence) but are counted as `unauthorized`. Malformed UUIDs raise before any emission point and are never counted. Counts backend requests that trigger a browser GET to the object store (one per cache miss), so it is a proxy for object GETs, not an exact count. +- The physical and logical counters intentionally overlap in coverage but differ in meaning; do not sum them. +- Metrics is not optional: `::mtx/metrics` is required by the storage and s3-backend schemas, and the assets handler cfg always carries it. Wiring a component without metrics is a bug, not a supported mode. +- Recording never fails: `app.metrics/run!` is safe by default at every emit point (`emit-op!`, `emit-dedup!`, `emit-asset!`, and the three S3 publisher emissions). The first recording failure per metric id logs at `warn`, later ones at `debug` (no log flood). The `instance` precondition is a plain assert and the collector lookup is outside the recording guard, so a missing instance fails hard (see `mem:backend/subtleties`). The `publish` outer try/catch stays: it is an SDK `MetricPublisher` contract boundary, not a metrics guard. diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index e051a8de74..354b9291a9 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -45,6 +45,10 @@ - The xnio worker MXBean can return transient `-1` (e.g. busy-thread count); negative samples are discarded (gauge keeps its previous value). Undertow exposes absolute request/error totals, so the sampler keeps a watermark atom and publishes deltas; a counter reset (decreasing totals) skips the negative delta and moves the watermark forward. - The `process_*` families (`process_open_fds`, `process_max_fds`, `process_cpu_seconds_total`, …) come from the prometheus client `StandardExports`, registered by `app.metrics/create-registry`. They read the OS MXBean reflectively and need the `jdk.management` module: on a pruned `jlink` JRE the MXBean is `sun.management.BaseOperatingSystemImpl`, the getters throw `NoSuchMethodException` and `StandardExports#collect` swallows it, so those families silently vanish from `/metrics`. `docker/images/Dockerfile.backend` keeps `jdk.management` in the `--add-modules` list, and `backend-tests.metrics-test` pins the contract. +## Metrics recording + +- `app.metrics/run!` is safe by default: a recording failure never throws (a metrics bug must not change the behavior of the measured operation). The first failure per metric id logs at `warn`, later ones at `debug`. The `instance` precondition is a plain assert (the backend enables `:backend-asserts`), and the collector lookup sits outside the recording guard, so a missing instance fails hard even when asserts are disabled. `::mtx/metrics` is required by the storage, s3-backend, and db-pool schemas; `app.db` wires the prometheus `MetricsTrackerFactory` unconditionally. Storage-specific metric contracts: `mem:backend/storage`. + ## Storage and media - Storage abstraction, backend configuration, logical buckets, object lifecycle, deduplication, access rules, and garbage collection: `mem:backend/storage`. diff --git a/.serena/memories/prod-infra/core.md b/.serena/memories/prod-infra/core.md index ee7dc542b0..8d6312562e 100644 --- a/.serena/memories/prod-infra/core.md +++ b/.serena/memories/prod-infra/core.md @@ -31,3 +31,4 @@ Penpot in production lives with both: horizontal-scale deployments accept "exact - Devenv composition and the ws0-only worker placement: `mem:devenv/core`. - Storage backend resolution, dedup, bucket behavior, object lifecycle, and file-data lifecycle: `mem:backend/storage`. +- Storage operation metrics (S3 API calls, logical ops, dedup, asset requests) and the logical-bucket vs physical-target distinction: `mem:backend/storage` (Metrics section). diff --git a/backend/src/app/db.clj b/backend/src/app/db.clj index 8f7719d705..d34911ff32 100644 --- a/backend/src/app/db.clj +++ b/backend/src/app/db.clj @@ -65,7 +65,8 @@ [::password {:optional true} :string] [::username {:optional true} :string] [::validation-timeout {:optional true} ::sm/int] - [::read-only {:optional true} ::sm/boolean]]) + [::read-only {:optional true} ::sm/boolean] + [::mtx/metrics ::mtx/metrics]]) (def defaults {::name :main @@ -130,11 +131,9 @@ (.setConnectionInitSql initsql) (.setInitializationFailTimeout -1)) - ;; When metrics namespace is provided - (when-let [instance (::mtx/metrics cfg)] - (->> (mtx/get-registry instance) - (PrometheusMetricsTrackerFactory.) - (.setMetricsTrackerFactory config))) + (->> (mtx/get-registry (::mtx/metrics cfg)) + (PrometheusMetricsTrackerFactory.) + (.setMetricsTrackerFactory config)) (some->> ^String (::username cfg) (.setUsername config)) (some->> ^String (::password cfg) (.setPassword config)) diff --git a/backend/src/app/http/assets.clj b/backend/src/app/http/assets.clj index 9ae258d2a2..e2c41d0886 100644 --- a/backend/src/app/http/assets.clj +++ b/backend/src/app/http/assets.clj @@ -15,6 +15,7 @@ [app.db :as db] [app.http.access-token :as actoken] [app.http.session :as session] + [app.metrics :as mtx] [app.rpc.permissions :as perms] [app.storage :as sto] [integrant.core :as ig] @@ -60,6 +61,28 @@ [pool id] (db/get* pool :file-media-object {:id id} {::db/remove-deleted false})) +(defn- result-label + "Map a response status to the outcome label. A nil or non-number + status is an `error`: every serve path must set ::yres/status." + [status] + (cond + (not (number? status)) "error" + (< status 400) "served" + (contains? #{401 403} status) "unauthorized" + (= status 404) "not-found" + :else "error")) + +(defn- emit-asset! + "Record an asset request. `route` is the handler route, `obj` the resolved + storage object (or nil when it could not be resolved). Recording never fails." + [cfg route obj status] + (mtx/run! (::mtx/metrics cfg) + :id :storage-asset-requests :inc 1 + :labels [route + (mtx/label (some-> obj :backend) "unknown") + (mtx/label (some-> obj meta :bucket) "unknown") + (result-label status)])) + (defn- serve-object-from-s3 [{:keys [::sto/storage ::signature-max-age ::cache-max-age] :as cfg} obj] (let [sig-max-age (or signature-max-age default-signature-max-age) @@ -104,7 +127,10 @@ [cfg {:keys [backend] :as obj}] (case backend (:s3 :assets-s3) (serve-object-from-s3 cfg obj) - (:fs :assets-fs) (serve-object-from-fs cfg obj))) + (:fs :assets-fs) (serve-object-from-fs cfg obj) + (ex/raise :type :internal + :hint "unknown storage backend" + :backend backend))) (defn- requires-auth? "Check if the storage object requires authentication based on its bucket." @@ -133,6 +159,18 @@ (or (nil? stored-profile-id) (= stored-profile-id request-profile-id)))) +(defn- serve-object-measured + "Serve `obj`, recording one asset metric per outcome. A failure is + counted and then rethrown: never swallowed, never counted twice." + [cfg route obj] + (try + (let [response (serve-object cfg obj)] + (emit-asset! cfg route obj (::yres/status response)) + response) + (catch Throwable cause + (emit-asset! cfg route obj 500) + (throw cause)))) + (defn objects-handler "Handler that serves storage objects by id. For non-public buckets (e.g. profile), requires authentication @@ -143,49 +181,65 @@ obj (sto/get-object storage id)] (cond (nil? obj) - {::yres/status 404} + (do + (emit-asset! cfg "by-id" nil 404) + {::yres/status 404}) (and (requires-auth? obj) (not (authenticated? request))) - {::yres/status 401} + (do + (emit-asset! cfg "by-id" obj 401) + {::yres/status 401}) + ;; The response stays 404 to avoid leaking existence, but the + ;; metric records the internal 401 outcome. (and (= (-> obj meta :bucket) sto/tempfile-bucket) (not (tempfile-owner-match? obj request))) - {::yres/status 404} + (do + (emit-asset! cfg "by-id" obj 401) + {::yres/status 404}) :else - (serve-object cfg obj)))) + (serve-object-measured cfg "by-id" obj)))) (defn- generic-handler "A generic handler helper/common code for file-media based handlers." - [{:keys [::sto/storage] :as cfg} request kf] + [{:keys [::sto/storage] :as cfg} request route kf] (let [pool (::db/pool storage) id (get-id request) mobj (get-file-media-object pool id)] (if (nil? mobj) - {::yres/status 404} + (do + (emit-asset! cfg route nil 404) + {::yres/status 404}) (let [file-id (:file-id mobj) profile-id (or (::session/profile-id request) (::actoken/profile-id request)) share-id (get-share-id request) perms (perms/get-file-read-permissions pool profile-id file-id share-id)] (if-not (:can-read perms) - {::yres/status 404} + ;; The response stays 404 to avoid leaking existence, but the + ;; metric records the internal 403 outcome. + (do + (emit-asset! cfg route nil 403) + {::yres/status 404}) (let [sobj (sto/get-object storage (kf mobj))] (if sobj - (serve-object cfg sobj) - {::yres/status 404}))))))) + (serve-object-measured cfg route sobj) + (do + (emit-asset! cfg route nil 404) + {::yres/status 404})))))))) (defn file-objects-handler "Handler that serves storage objects by file media id." [cfg request] - (generic-handler cfg request :media-id)) + (generic-handler cfg request "by-file-media-id" :media-id)) (defn file-thumbnails-handler "Handler that serves storage objects by thumbnail-id and quick fallback to file-media-id if no thumbnail is available." [cfg request] - (generic-handler cfg request #(or (:thumbnail-id %) (:media-id %)))) + (generic-handler cfg request "thumbnail" #(or (:thumbnail-id %) (:media-id %)))) ;; --- Initialization diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index 27e34e4ad8..6bd8da3d13 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -143,6 +143,43 @@ ::mdef/labels [] ::mdef/type :histogram} + :storage-s3-requests + {::mdef/name "penpot_storage_s3_requests_total" + ::mdef/help "Total S3 API calls performed by the storage backend." + ::mdef/labels ["operation" "target" "result"] + ::mdef/type :counter} + + :storage-s3-retries + {::mdef/name "penpot_storage_s3_retries_total" + ::mdef/help "Total SDK retries observed on S3 API calls." + ::mdef/labels ["operation" "target"] + ::mdef/type :counter} + + :storage-s3-timing + {::mdef/name "penpot_storage_s3_timing" + ::mdef/help "S3 API call timing (milliseconds)." + ::mdef/labels ["operation" "target"] + ::mdef/type :histogram + ::mdef/buckets [5 10 25 50 100 250 500 1000 2500 5000 10000 30000 60000]} + + :storage-operations + {::mdef/name "penpot_storage_operations_total" + ::mdef/help "Logical storage operations by Penpot bucket." + ::mdef/labels ["op" "bucket" "backend"] + ::mdef/type :counter} + + :storage-dedup + {::mdef/name "penpot_storage_dedup_total" + ::mdef/help "Storage deduplication outcomes." + ::mdef/labels ["result" "bucket"] + ::mdef/type :counter} + + :storage-asset-requests + {::mdef/name "penpot_storage_asset_requests_total" + ::mdef/help "Asset requests served by app.http.assets." + ::mdef/labels ["route" "backend" "bucket" "result"] + ::mdef/type :counter} + :http-server-dispatch-timing {::mdef/name "penpot_http_server_dispatch_timing" ::mdef/help "Histogram of dispatch handler" @@ -345,6 +382,7 @@ {::http.assets/path (cf/get :assets-path) ::http.assets/cache-max-age (ct/duration {:hours 24}) ::http.assets/signature-max-age (ct/duration {:hours 24 :minutes 15}) + ::mtx/metrics (ig/ref ::mtx/metrics) ::sto/storage (ig/ref ::sto/storage) ::session/manager (ig/ref ::session/manager) ::setup/props (ig/ref ::setup/props) @@ -550,6 +588,7 @@ ::sto/storage {::db/pool (ig/ref ::db/pool) + ::mtx/metrics (ig/ref ::mtx/metrics) ::sto/backends {:s3 (ig/ref :app.storage.s3/backend) :fs (ig/ref :app.storage.fs/backend) @@ -569,6 +608,7 @@ (cf/get :objects-storage-s3-bucket)) ::sto.s3/io-threads (or (cf/get :storage-assets-s3-io-threads) (cf/get :objects-storage-s3-io-threads)) + ::mtx/metrics (ig/ref ::mtx/metrics) ::wrk/netty-io-executor (ig/ref ::wrk/netty-io-executor)} diff --git a/backend/src/app/metrics.clj b/backend/src/app/metrics.clj index 63145eb5b5..aff1e846f5 100644 --- a/backend/src/app/metrics.clj +++ b/backend/src/app/metrics.clj @@ -58,11 +58,17 @@ (def ^:private schema:definitions [:map-of :keyword - [:map {:title "definition"} + [:map {:title "definition" :closed true} [::mdef/name :string] [::mdef/help :string] [::mdef/type [:enum :gauge :counter :summary :histogram]] [::mdef/labels {:optional true} [::sm/vec :string]] + [::mdef/quantiles {:optional true} [::sm/vec [:tuple :double :double]]] + [::mdef/max-age {:optional true} :int] + ;; NB: in :summary, buckets are the age buckets; in :histogram, + ;; the observation buckets. + [::mdef/buckets {:optional true} [::sm/vec [:or :int :double]]] + [::mdef/reg {:optional true} ::registry] [::mdef/instance {:optional true} ::collector]]]) (defn metrics? @@ -131,15 +137,51 @@ (def default-histogram-buckets [1 5 10 25 50 75 100 250 500 750 1000 2500 5000 7500]) +(defn label + "Coerce a metric label value to string, falling back when absent." + [value fallback] + (cond + (string? value) value + (keyword? value) (name value) + (number? value) (str value) + :else fallback)) + (defmulti run-collector! (fn [mdef _] (::mdef/type mdef))) (defmulti create-collector ::mdef/type) +(defonce ^:private warned-hints (atom #{})) + +(defn- report-safe-failure! + [hint cause] + (if (contains? @warned-hints hint) + (l/dbg :hint hint :cause cause) + (do + (swap! warned-hints conj hint) + (l/wrn :hint hint :cause cause)))) + (defn run! + "Record a metric. + + Recording never throws: a metrics bug must not change the behavior of + the operation being measured. The first failure per metric id logs at + warn level and later ones at debug, so a broken setup surfaces once + without flooding the log on every request. + + The `instance` precondition is a plain assert: it holds because every + component is wired with metrics (`::mtx/metrics` is required by the + component schemas). The collector lookup stays outside the recording + guard, so a missing instance also fails hard when asserts are + disabled." [instance & {:keys [id] :as params}] (assert (metrics? instance) "expected valid metrics instance") + (when-let [mobj (get-collector instance id)] - (run-collector! mobj params) - true)) + (try + (run-collector! mobj params) + true + (catch Throwable cause + (report-safe-failure! (str "unable to record metric " (pr-str id)) cause) + nil)))) (defn- create-registry [] diff --git a/backend/src/app/storage.clj b/backend/src/app/storage.clj index 71dabca288..52163bfc95 100644 --- a/backend/src/app/storage.clj +++ b/backend/src/app/storage.clj @@ -16,6 +16,7 @@ [app.common.uuid :as uuid] [app.config :as cf] [app.db :as db] + [app.metrics :as mtx] [app.storage.fs :as sfs] [app.storage.impl :as impl] [app.storage.s3 :as ss3] @@ -75,6 +76,7 @@ [:map {:title "storage"} [::backends schema:backends] [::backend [:enum :s3 :fs]] + [::mtx/metrics ::mtx/metrics] ::db/pool]) (def valid-storage? @@ -184,6 +186,26 @@ (dm/export impl/wrap-with-hash) (dm/export impl/object?) +(defn- emit-op! + "Record a logical storage operation. Recording never fails: metrics + must not change storage behavior." + ([storage op bucket] + (emit-op! storage op bucket nil)) + ([storage op bucket object] + (mtx/run! (::mtx/metrics storage) + :id :storage-operations :inc 1 + :labels [op + (mtx/label bucket "unknown") + (mtx/label (or (some-> object :backend) (::backend storage)) + "unknown")]))) + +(defn- emit-dedup! + "Record a deduplication outcome. Recording never fails." + [storage result bucket] + (mtx/run! (::mtx/metrics storage) + :id :storage-dedup :inc 1 + :labels [(name result) (mtx/label bucket "unknown")])) + (defn get-object [storage id] (assert (valid-storage? storage)) @@ -206,28 +228,35 @@ (or touched-at (ct/now)) touched-at) - backend' (impl/resolve-backend storage backend)] + backend' (impl/resolve-backend storage backend) + + bucket (:bucket mdata) + dedupable? (and (::deduplicate? params) + (:hash mdata) + (some? bucket) + (not= tempfile-bucket bucket) + (not= upload-session-bucket bucket)) + + hit (when dedupable? + (get-database-object-by-hash pool backend bucket (:hash mdata)))] ;; NOTE: for now we don't reuse the deleted objects, but in futute ;; we can consider reusing deleted objects if we found a duplicated ;; one and is marked for deletion but still not deleted. - ;; PHASE 1: deduplication lookup. - (if-some [hit (when (and (::deduplicate? params) - (:hash mdata) - (:bucket mdata) - (not= tempfile-bucket (:bucket mdata)) - (not= upload-session-bucket (:bucket mdata))) - (get-database-object-by-hash pool backend - (:bucket mdata) - (:hash mdata)))] + ;; PHASE 1: deduplication lookup (see `dedupable?` and `hit` above). + (if-some [hit hit] ;; PHASE 2: an existing reference is found: reuse or repair it. + ;; The `exists` op is emitted only after a successful probe so every + ;; count stays paired with its `hit`/`repair` outcome. (if (impl/exists-object? backend' hit) ;; PHASE 2a: healthy reference. Optionally refresh touched_at ;; and reuse the object as it is. (do + (emit-op! storage "exists" bucket) + (emit-dedup! storage :hit bucket) (when touch (db/update! pool :storage-object {:touched-at touched-at} @@ -246,6 +275,9 @@ :id (:id object) :backend (name backend)) (impl/put-object backend' object content) + (emit-op! storage "exists" bucket) + (emit-op! storage "repair" bucket) + (emit-dedup! storage :repair bucket) (promote-object! storage object) object)) @@ -263,20 +295,24 @@ :status "pending"}) object (row->storage-object row)] (impl/put-object backend' object content) + (emit-op! storage "put" bucket) + (emit-dedup! storage (if dedupable? :miss :skip) bucket) (promote-object! storage object) object)))) (defn touch-object! - "Mark object as touched." - [storage object-or-id] + "Mark object as touched. Takes the object id (UUID). The metric labels + come from the updated row itself (RETURNING); no row, no metric." + [storage id] (assert (valid-storage? storage)) - (let [id (if (impl/object? object-or-id) (:id object-or-id) object-or-id) - ds (db/get-connectable storage)] - (-> (db/update! ds :storage-object - {:touched-at (ct/now)} - {:id id}) - (db/get-update-count) - (pos?)))) + (let [ds (db/get-connectable storage) + res (db/update! ds :storage-object + {:touched-at (ct/now)} + {:id id} + {::db/return-keys [:id :backend :metadata]})] + (when-some [object (some-> res row->storage-object)] + (emit-op! storage "touch" (-> object meta :bucket) object)) + (some? res))) (defn get-object-data "Return an input stream instance of the object content." @@ -285,8 +321,10 @@ (assert (valid-storage? storage)) (when (or (nil? (:expired-at object)) (ct/is-after? (:expired-at object) (ct/now))) - (-> (impl/resolve-backend storage (:backend object)) - (impl/get-object-data object)))) + (let [result (-> (impl/resolve-backend storage (:backend object)) + (impl/get-object-data object))] + (emit-op! storage "get-data" (-> object meta :bucket) object) + result))) (defn get-object-bytes "Returns a byte array of object content." @@ -294,8 +332,10 @@ (assert (valid-storage? storage)) (when (or (nil? (:expired-at object)) (ct/is-after? (:expired-at object) (ct/now))) - (-> (impl/resolve-backend storage (:backend object)) - (impl/get-object-bytes object)))) + (let [result (-> (impl/resolve-backend storage (:backend object)) + (impl/get-object-bytes object))] + (emit-op! storage "get-bytes" (-> object meta :bucket) object) + result))) (defn get-object-url ([storage object] @@ -319,14 +359,21 @@ (-> (impl/get-object-url backend object nil) file-url->path)))) (defn del-object! - [storage object-or-id] + "Mark the object as deleted (soft delete: the backend content is + removed by the GC). Takes the object id (UUID). Only a live row + (deleted_at IS NULL) is deleted, so a repeated call is a no-op that + returns false. The metric labels come from the updated row itself + (RETURNING); no row, no metric." + [storage id] (assert (valid-storage? storage)) - (let [id (if (impl/object? object-or-id) (:id object-or-id) object-or-id) - ds (db/get-connectable storage) + (let [ds (db/get-connectable storage) res (db/update! ds :storage-object {:deleted-at (ct/now)} - {:id id})] - (pos? (db/get-update-count res)))) + ["id = ? AND deleted_at IS NULL" id] + {::db/return-keys [:id :backend :metadata]})] + (when-some [object (some-> res row->storage-object)] + (emit-op! storage "del" (-> object meta :bucket) object)) + (some? res))) (dm/export impl/calculate-hash) (dm/export impl/get-hash) diff --git a/backend/src/app/storage/s3.clj b/backend/src/app/storage/s3.clj index 6cf97321f1..cde4afc3c5 100644 --- a/backend/src/app/storage/s3.clj +++ b/backend/src/app/storage/s3.clj @@ -14,8 +14,10 @@ [app.common.schema :as sm] [app.common.time :as ct] [app.common.uri :as u] + [app.metrics :as mtx] [app.storage :as-alias sto] [app.storage.impl :as impl] + [app.storage.s3.metrics :as s3m] [app.storage.tmp :as tmp] [app.worker :as-alias wrk] [clojure.java.io :as io] @@ -38,9 +40,11 @@ software.amazon.awssdk.core.async.AsyncResponseTransformer software.amazon.awssdk.core.async.BlockingInputStreamAsyncRequestBody software.amazon.awssdk.core.client.config.ClientAsyncConfiguration + software.amazon.awssdk.core.client.config.ClientOverrideConfiguration software.amazon.awssdk.core.ResponseBytes software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient software.amazon.awssdk.http.nio.netty.SdkEventLoopGroup + software.amazon.awssdk.metrics.MetricPublisher software.amazon.awssdk.regions.Region software.amazon.awssdk.services.s3.model.Delete software.amazon.awssdk.services.s3.model.DeleteObjectRequest @@ -94,7 +98,8 @@ [::region {:optional true} :keyword] [::bucket {:optional true} ::sm/text] [::prefix {:optional true} ::sm/text] - [::endpoint {:optional true} ::sm/uri]]) + [::endpoint {:optional true} ::sm/uri] + [::mtx/metrics ::mtx/metrics]]) (defmethod ig/expand-key ::backend [k v] @@ -237,11 +242,21 @@ (Region/of (name region))) (defn- build-s3-client - [{:keys [::region ::endpoint ::wrk/netty-io-executor]}] + [{:keys [::region ::endpoint ::wrk/netty-io-executor ::mtx/metrics]}] + + ;; TODO: per-bucket S3 routing will introduce more physical targets; + ;; until then the target label is always :default. Do not add more + ;; target ids here until that plan lands. (let [creds-provider (DefaultCredentialsProvider/create) + publisher (s3m/wrap-publisher metrics :default) aconfig (-> (ClientAsyncConfiguration/builder) (.build)) + oconfig (let [builder (ClientOverrideConfiguration/builder)] + (.addMetricPublisher ^software.amazon.awssdk.core.client.config.ClientOverrideConfiguration$Builder builder + ^MetricPublisher publisher) + (.build ^software.amazon.awssdk.core.client.config.ClientOverrideConfiguration$Builder builder)) + sconfig (-> (S3Configuration/builder) (cond-> (some? endpoint) (.pathStyleAccessEnabled true)) (.build)) @@ -259,6 +274,7 @@ client (let [builder (S3AsyncClient/builder) builder (.serviceConfiguration ^S3AsyncClientBuilder builder ^S3Configuration sconfig) builder (.asyncConfiguration ^S3AsyncClientBuilder builder ^ClientAsyncConfiguration aconfig) + builder (.overrideConfiguration ^S3AsyncClientBuilder builder ^ClientOverrideConfiguration oconfig) builder (.httpClient ^S3AsyncClientBuilder builder ^NettyNioAsyncHttpClient hclient) builder (.region ^S3AsyncClientBuilder builder (lookup-region region)) builder (.credentialsProvider ^S3AsyncClientBuilder builder creds-provider) diff --git a/backend/src/app/storage/s3/metrics.clj b/backend/src/app/storage/s3/metrics.clj new file mode 100644 index 0000000000..1ceb4cec24 --- /dev/null +++ b/backend/src/app/storage/s3/metrics.clj @@ -0,0 +1,79 @@ +;; 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 app.storage.s3.metrics + "Prometheus metrics for physical S3 API calls." + (:require + [app.common.logging :as l] + [app.metrics :as mtx]) + (:import + java.time.Duration + software.amazon.awssdk.core.metrics.CoreMetric + software.amazon.awssdk.metrics.MetricCollection + software.amazon.awssdk.metrics.MetricPublisher)) + +(defn- operation-label + [operation] + (mtx/label operation "unknown")) + +(defn- target-label + [target] + (mtx/label target "default")) + +(defn- result-label + [successful?] + (if (true? successful?) "ok" "error")) + +(defn- retries-count + [retries] + (try + (long (or retries 0)) + (catch Throwable _ 0))) + +(defn- duration-millis + [duration] + (when (instance? Duration duration) + (.toMillis ^Duration duration))) + +(defn- first-value + [^MetricCollection collection metric] + (first (.metricValues collection metric))) + +(defn- record-collection! + [metrics target ^MetricCollection collection] + (let [operation (operation-label (first-value collection CoreMetric/OPERATION_NAME))] + (if (= operation "unknown") + (l/wrn :hint "ignoring s3 metric without operation name") + (let [ok? (first-value collection CoreMetric/API_CALL_SUCCESSFUL) + retries (retries-count (first-value collection CoreMetric/RETRY_COUNT)) + duration (duration-millis (first-value collection CoreMetric/API_CALL_DURATION))] + (mtx/run! metrics + :id :storage-s3-requests :inc 1 + :labels [operation target (result-label ok?)]) + (when (pos? retries) + (mtx/run! metrics + :id :storage-s3-retries :inc retries + :labels [operation target])) + (when (some? duration) + (mtx/run! metrics + :id :storage-s3-timing :val duration + :labels [operation target])))))) + +(defn wrap-publisher + "Return a MetricPublisher that records each S3 API call. + + `target` is the physical storage target id. `metrics` is required by + the s3 backend schema." + [metrics target] + (let [target (target-label target)] + (reify MetricPublisher + (^void publish [_ ^MetricCollection collection] + (try + (record-collection! metrics target collection) + (catch Throwable cause + (l/dbg :hint "unable to record s3 metric" :cause cause))) + nil) + (^void close [_])))) diff --git a/backend/test/backend_tests/db_test.clj b/backend/test/backend_tests/db_test.clj index 39388dd825..29d3095f32 100644 --- a/backend/test/backend_tests/db_test.clj +++ b/backend/test/backend_tests/db_test.clj @@ -9,7 +9,8 @@ [app.common.uuid :as uuid] [app.db :as db] [backend-tests.helpers :as th] - [clojure.test :as t]) + [clojure.test :as t] + [integrant.core :as ig]) (:import com.zaxxer.hikari.HikariConfig com.zaxxer.hikari.HikariDataSource @@ -17,6 +18,15 @@ (t/use-fixtures :once th/state-init) +(t/deftest pool-requires-metrics + ;; Metrics is required by the pool schema: a pool wired without it + ;; fails at the boundary (AssertionError when asserts are enabled) or + ;; when the prometheus tracker is wired, instead of silently skipping + ;; the instrumentation. + (t/is (thrown? Throwable + (ig/init-key :app.db/pool + {::db/uri (java.net.URI. "postgresql://localhost/test")})))) + (t/deftest pool-stats-returns-expected-keys (let [stats (db/pool-stats th/*pool*)] (t/testing "all expected keys are present" diff --git a/backend/test/backend_tests/http_assets_test.clj b/backend/test/backend_tests/http_assets_test.clj index 6ebddef0f3..095d04f3e7 100644 --- a/backend/test/backend_tests/http_assets_test.clj +++ b/backend/test/backend_tests/http_assets_test.clj @@ -7,6 +7,7 @@ (ns backend-tests.http-assets-test (:require [app.common.time :as ct] + [app.common.uri :as u] [app.common.uuid :as uuid] [app.config :as cf] [app.db :as db] @@ -14,13 +15,21 @@ [app.http.access-token :as actoken] [app.http.assets :as assets] [app.http.session :as session] + [app.main :as main] + [app.metrics :as mtx] + [app.metrics.definition :as-alias mdef] [app.rpc :as-alias rpc] [app.rpc.commands.access-token :as access-token] [app.storage :as sto] [backend-tests.helpers :as th] [clojure.test :as t] [datoteka.fs :as fs] - [yetti.response :as-alias yres])) + [integrant.core :as ig] + [mockery.core :refer [with-mocks]] + [yetti.response :as-alias yres]) + (:import + io.prometheus.client.Counter + io.prometheus.client.Counter$Child)) (t/use-fixtures :once th/state-init) (t/use-fixtures :each (th/serial @@ -49,12 +58,32 @@ (some? profile-id) (assoc :profile-id profile-id))))) +(defn- make-metrics + [] + (ig/init-key :app.metrics/metrics + {:default (select-keys main/default-metrics + [:storage-asset-requests])})) + (defn- make-handler-cfg - "Build a minimal cfg map for the assets handlers." + "Build a minimal cfg map for the assets handlers. It carries a metrics + instance because the handlers require one." [storage] {::sto/storage storage + ::mtx/metrics (make-metrics) ::assets/path "/assets"}) +(defn- make-metrics-cfg + "Build a handler cfg map with an isolated metrics instance." + [storage metrics] + (assoc (make-handler-cfg storage) ::mtx/metrics metrics)) + +(defn- counter-value + [metrics labels] + (let [collector (mtx/get-collector metrics :storage-asset-requests) + instance (::mdef/instance collector) + child (.labels ^Counter instance (into-array String labels))] + (.get ^Counter$Child child))) + ;; ---------------------------------------------------------------- ;; Tests: get-id ;; ---------------------------------------------------------------- @@ -847,3 +876,319 @@ ::session/profile-id (:id stranger)} response (assets/objects-handler cfg request)] (t/is (= 204 (::yres/status response))))) + +;; ---------------------------------------------------------------- +;; Tests: asset request metrics +;; ---------------------------------------------------------------- + +(t/deftest objects-handler-emits-served-metric + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + object (create-storage-object! storage "file-media-object" "file content") + request {:path-params {:id (str (:id object))}} + response (assets/objects-handler cfg request)] + (t/is (= 204 (::yres/status response))) + (t/is (= 1.0 (counter-value metrics ["by-id" "fs" "file-media-object" "served"]))))) + +(t/deftest objects-handler-emits-not-found-metric + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + request {:path-params {:id (str (uuid/next))}} + response (assets/objects-handler cfg request)] + (t/is (= 404 (::yres/status response))) + (t/is (= 1.0 (counter-value metrics ["by-id" "unknown" "unknown" "not-found"]))))) + +(t/deftest objects-handler-emits-unauthorized-metric + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + object (create-storage-object! storage "profile" "profile photo") + request {:path-params {:id (str (:id object))}} + response (assets/objects-handler cfg request)] + (t/is (= 401 (::yres/status response))) + (t/is (= 1.0 (counter-value metrics ["by-id" "fs" "profile" "unauthorized"]))))) + +(t/deftest file-objects-handler-emits-route-metric + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + owner (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + media-storage (create-storage-object! storage "file-media-object" "image data") + media-obj (th/create-file-media-object* {:file-id (:id file) + :media-id (:id media-storage)}) + request {:path-params {:id (str (:id media-obj))} + ::session/profile-id (:id owner)} + response (assets/file-objects-handler cfg request)] + (t/is (= 204 (::yres/status response))) + (t/is (= 1.0 (counter-value metrics ["by-file-media-id" "fs" "file-media-object" "served"]))))) + +(t/deftest file-objects-handler-no-perms-emits-unauthorized-metric + ;; Permission-denied file-media requests answer 404 but are counted as + ;; unauthorized (no existence is leaked over HTTP). + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + owner (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + media-storage (create-storage-object! storage "file-media-object" "image data") + media-obj (th/create-file-media-object* {:file-id (:id file) + :media-id (:id media-storage)}) + stranger (th/create-profile* 2) + request {:path-params {:id (str (:id media-obj))} + ::session/profile-id (:id stranger)} + response (assets/file-objects-handler cfg request)] + (t/is (= 404 (::yres/status response))) + (t/is (= 1.0 (counter-value metrics ["by-file-media-id" "unknown" "unknown" "unauthorized"]))) + (t/is (= 0.0 (counter-value metrics ["by-file-media-id" "unknown" "unknown" "not-found"]))))) + +(t/deftest asset-requests-default-metrics-definition + (let [defs main/default-metrics] + (t/is (= "penpot_storage_asset_requests_total" (::mdef/name (:storage-asset-requests defs)))) + (t/is (= ["route" "backend" "bucket" "result"] (::mdef/labels (:storage-asset-requests defs)))))) + +(t/deftest objects-handler-serve-failure-emits-error-and-rethrows + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + object (create-storage-object! storage "file-media-object" "file content") + request {:path-params {:id (str (:id object))}}] + (with-mocks [_mock {:target 'app.storage/object->relative-path + :throw (ex-info "boom" {})}] + (t/is (thrown? clojure.lang.ExceptionInfo + (assets/objects-handler cfg request)))) + (t/is (= 1.0 (counter-value metrics ["by-id" "fs" "file-media-object" "error"]))) + (t/is (= 0.0 (counter-value metrics ["by-id" "fs" "file-media-object" "served"]))))) + +(t/deftest objects-handler-s3-backend-emits-served-metric + ;; The S3 path is exercised without a real object store: the row is + ;; inserted directly and the presigned URL is mocked. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + id (uuid/next)] + (db/insert! th/*pool* :storage-object + {:id id + :size 9 + :backend "s3" + :metadata (db/tjson {:bucket "file-media-object" + :content-type "text/plain"}) + :status "valid"}) + (with-mocks [_mock {:target 'app.storage/get-object-url + :return (fn [_ _ & _] (u/uri "https://example.invalid/object"))}] + (let [response (assets/objects-handler cfg {:path-params {:id (str id)}})] + (t/is (= 307 (::yres/status response))))) + (t/is (= 1.0 (counter-value metrics ["by-id" "s3" "file-media-object" "served"]))))) + +(t/deftest objects-handler-unknown-backend-raises-and-emits-error + ;; A row with an unexpected backend fails explicitly instead of + ;; returning nil to the router; the failure is counted and rethrown. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + id (uuid/next)] + (db/insert! th/*pool* :storage-object + {:id id + :size 9 + :backend "bogus" + :metadata (db/tjson {:bucket "file-media-object" + :content-type "text/plain"}) + :status "valid"}) + (t/is (thrown? clojure.lang.ExceptionInfo + (assets/objects-handler cfg {:path-params {:id (str id)}}))) + (t/is (= 1.0 (counter-value metrics ["by-id" "bogus" "file-media-object" "error"]))))) + +(t/deftest file-thumbnails-handler-emits-thumbnail-route-metric + ;; Served through the real thumbnail-id path (not the media-id fallback). + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + owner (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + media-storage (create-storage-object! storage "file-media-object" "image data") + thumb-storage (create-storage-object! storage "file-object-thumbnail" "thumb data") + media-obj (th/create-file-media-object* {:file-id (:id file) + :media-id (:id media-storage)})] + (th/db-update! :file-media-object + {:thumbnail-id (:id thumb-storage)} + {:id (:id media-obj)}) + (let [request {:path-params {:id (str (:id media-obj))} + ::session/profile-id (:id owner)} + response (assets/file-thumbnails-handler cfg request)] + (t/is (= 204 (::yres/status response))) + (t/is (= 1.0 (counter-value metrics ["thumbnail" "fs" "file-object-thumbnail" "served"])))))) + +(t/deftest file-thumbnails-handler-fallback-emits-thumbnail-route-metric + ;; Served through the media-id fallback (no thumbnail-id set). + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + owner (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + media-storage (create-storage-object! storage "file-media-object" "image data") + media-obj (th/create-file-media-object* {:file-id (:id file) + :media-id (:id media-storage)}) + request {:path-params {:id (str (:id media-obj))} + ::session/profile-id (:id owner)} + response (assets/file-thumbnails-handler cfg request)] + (t/is (= 204 (::yres/status response))) + (t/is (= 1.0 (counter-value metrics ["thumbnail" "fs" "file-media-object" "served"]))))) + +(t/deftest file-thumbnails-handler-missing-storage-emits-not-found-metric + ;; Media row exists but the storage object is gone: 404 with unknown labels. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + owner (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + media-storage (create-storage-object! storage "file-media-object" "image data") + media-obj (th/create-file-media-object* {:file-id (:id file) + :media-id (:id media-storage)}) + request {:path-params {:id (str (:id media-obj))} + ::session/profile-id (:id owner)}] + (with-mocks [_mock {:target 'app.storage/get-object + :return (fn [_ _] nil)}] + (let [response (assets/file-thumbnails-handler cfg request)] + (t/is (= 404 (::yres/status response))))) + (t/is (= 1.0 (counter-value metrics ["thumbnail" "unknown" "unknown" "not-found"]))))) + +(t/deftest objects-handler-tempfile-mismatch-emits-unauthorized-metric + ;; The response stays 404 to avoid leaking existence, but the metric + ;; records the internal auth outcome. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + owner (th/create-profile* 1) + stranger (th/create-profile* 2) + object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)}) + request {:path-params {:id (str (:id object))} + ::session/profile-id (:id stranger)} + response (assets/objects-handler cfg request)] + (t/is (= 404 (::yres/status response))) + (t/is (= 1.0 (counter-value metrics ["by-id" "fs" "tempfile" "unauthorized"]))) + (t/is (= 0.0 (counter-value metrics ["by-id" "fs" "tempfile" "not-found"]))))) + +(t/deftest handlers-require-metrics + ;; Metrics is no longer optional: a handler wired without it must fail + ;; loudly instead of silently dropping the measurement. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + cfg (dissoc (make-handler-cfg storage) ::mtx/metrics) + owner (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + object (create-storage-object! storage "file-media-object" "file content") + media-obj (th/create-file-media-object* {:file-id (:id file) + :media-id (:id object)})] + (t/is (thrown? Throwable + (assets/objects-handler + cfg {:path-params {:id (str (:id object))}}))) + (t/is (thrown? Throwable + (assets/file-objects-handler + cfg {:path-params {:id (str (:id media-obj))} + ::session/profile-id (:id owner)}))) + (t/is (thrown? Throwable + (assets/file-thumbnails-handler + cfg {:path-params {:id (str (:id media-obj))} + ::session/profile-id (:id owner)}))) + (t/is (thrown? Throwable + (assets/objects-handler + cfg {:path-params {:id (str (uuid/next))}}))))) + +(t/deftest malformed-uuid-emits-nothing + ;; get-id raises before any metric emission point is reached. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + request {:path-params {:id "not-a-uuid"}}] + (t/is (thrown? clojure.lang.ExceptionInfo + (assets/objects-handler cfg request))) + (t/is (= 0.0 (counter-value metrics ["by-id" "fs" "file-media-object" "served"]))) + (t/is (= 0.0 (counter-value metrics ["by-id" "unknown" "unknown" "not-found"]))) + (t/is (= 0.0 (counter-value metrics ["by-id" "unknown" "unknown" "error"]))))) + +(t/deftest assets-handler-survives-metrics-failure + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + object (create-storage-object! storage "file-media-object" "file content") + request {:path-params {:id (str (:id object))}}] + (with-mocks [_mock {:target 'app.metrics/run-collector! + :throw (ex-info "boom" {})}] + (let [response (assets/objects-handler cfg request)] + (t/is (= 204 (::yres/status response))))))) + +(t/deftest file-objects-handler-serve-failure-emits-error-and-rethrows + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + metrics (make-metrics) + cfg (make-metrics-cfg storage metrics) + owner (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + media-storage (create-storage-object! storage "file-media-object" "image data") + media-obj (th/create-file-media-object* {:file-id (:id file) + :media-id (:id media-storage)}) + request {:path-params {:id (str (:id media-obj))} + ::session/profile-id (:id owner)}] + (with-mocks [_mock {:target 'app.storage/object->relative-path + :throw (ex-info "boom" {})}] + (t/is (thrown? clojure.lang.ExceptionInfo + (assets/file-objects-handler cfg request)))) + (t/is (= 1.0 (counter-value metrics ["by-file-media-id" "fs" "file-media-object" "error"]))) + (t/is (= 0.0 (counter-value metrics ["by-file-media-id" "fs" "file-media-object" "served"]))))) + +(t/deftest result-label-mapping + (t/are [status expected] + (= expected (#'app.http.assets/result-label status)) + nil "error" + 200 "served" + 204 "served" + 307 "served" + 401 "unauthorized" + 403 "unauthorized" + 404 "not-found" + 429 "error" + 500 "error")) diff --git a/backend/test/backend_tests/metrics_test.clj b/backend/test/backend_tests/metrics_test.clj index f447ca05e5..5365ce1932 100644 --- a/backend/test/backend_tests/metrics_test.clj +++ b/backend/test/backend_tests/metrics_test.clj @@ -7,11 +7,15 @@ (ns backend-tests.metrics-test (:require [app.metrics :as mtx] + [app.metrics.definition :as-alias mdef] [clojure.test :as t] [integrant.core :as ig]) (:import io.prometheus.client.Collector$MetricFamilySamples - io.prometheus.client.Collector$MetricFamilySamples$Sample)) + io.prometheus.client.Collector$MetricFamilySamples$Sample + io.prometheus.client.CollectorRegistry)) + +(def ^:private valid-definitions? @#'app.metrics/valid-definitions?) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Helpers @@ -47,3 +51,70 @@ (t/is (contains? names "process_open_fds")) (t/is (contains? names "process_max_fds")) (t/is (contains? names "process_cpu_seconds_total")))) + +(t/deftest label-coercion + (t/are [value fallback expected] + (= expected (mtx/label value fallback)) + "fs" "unknown" "fs" + :s3 "unknown" "s3" + 200 "unknown" "200" + nil "unknown" "unknown" + nil "default" "default")) + +(t/deftest run-contains-label-arity-bug + ;; A wrong label count throws inside the prometheus client; the + ;; recording failure must not propagate to the caller. + (let [registry (CollectorRegistry.) + collector (mtx/create-collector {::mdef/name "penpot_test_run_arity" + ::mdef/help "test helper" + ::mdef/type :counter + ::mdef/labels ["a" "b"] + :app.metrics/registry registry}) + instance (reify mtx/IMetrics + (get-collector [_ _] collector))] + (t/is (nil? (mtx/run! instance :id :x :inc 1 :labels ["only-one"]))))) + +(t/deftest run-fails-loudly-without-metrics-instance + ;; A missing or invalid instance is a wiring bug: it must fail on every + ;; invocation instead of silently dropping the measurement. With + ;; asserts enabled this is an AssertionError; with asserts disabled the + ;; collector lookup (outside the recording guard) throws instead. + (t/is (thrown? Throwable (mtx/run! nil :id :x :inc 1))) + (t/is (thrown? Throwable (mtx/run! :not-metrics :id :x :inc 1)))) + +(t/deftest run-records-on-success + (let [registry (CollectorRegistry.) + collector (mtx/create-collector {::mdef/name "penpot_test_run" + ::mdef/help "test helper" + ::mdef/type :counter + ::mdef/labels ["result"] + :app.metrics/registry registry}) + instance (reify mtx/IMetrics + (get-collector [_ _] collector))] + (t/is (true? (mtx/run! instance :id :x :inc 1 :labels ["ok"]))))) + +(t/deftest definitions-schema-accepts-all-consumed-keys + (t/is (true? (valid-definitions? + {:test-timing + {::mdef/name "penpot_test_timing" + ::mdef/help "test" + ::mdef/type :histogram + ::mdef/labels ["op"] + ::mdef/buckets [5 10 25]} + :test-summary + {::mdef/name "penpot_test_summary" + ::mdef/help "test" + ::mdef/type :summary + ::mdef/quantiles [[0.5 0.01]] + ::mdef/max-age 60}})))) + + +(t/deftest definitions-schema-rejects-unknown-keys + ;; A typo such as ::mdef/bucksets must fail at startup instead of + ;; silently falling back to the default histogram buckets. The + ;; definition map is closed, so unknown keys do not validate. + (t/is (false? (valid-definitions? + {:bad {::mdef/name "penpot_bad_metric" + ::mdef/help "typo check" + ::mdef/type :histogram + ::mdef/bucksets [100]}})))) diff --git a/backend/test/backend_tests/rpc_management_test.clj b/backend/test/backend_tests/rpc_management_test.clj index 1148fde042..570b858aaf 100644 --- a/backend/test/backend_tests/rpc_management_test.clj +++ b/backend/test/backend_tests/rpc_management_test.clj @@ -178,7 +178,7 @@ :media-id (:id sobject)})] (th/mark-file-deleted* {:id (:id file2)}) - (sto/del-object! storage sobject) + (sto/del-object! storage (:id sobject)) (let [data {::th/type :duplicate-file ::rpc/profile-id (:id profile) diff --git a/backend/test/backend_tests/storage_metrics_test.clj b/backend/test/backend_tests/storage_metrics_test.clj new file mode 100644 index 0000000000..7cf660da03 --- /dev/null +++ b/backend/test/backend_tests/storage_metrics_test.clj @@ -0,0 +1,307 @@ +;; 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"]))))) diff --git a/backend/test/backend_tests/storage_s3_metrics_test.clj b/backend/test/backend_tests/storage_s3_metrics_test.clj new file mode 100644 index 0000000000..41f0c33d1d --- /dev/null +++ b/backend/test/backend_tests/storage_s3_metrics_test.clj @@ -0,0 +1,134 @@ +;; 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-s3-metrics-test + (:require + [app.main :as main] + [app.metrics :as mtx] + [app.metrics.definition :as-alias mdef] + [app.storage.s3.metrics :as s3m] + [clojure.test :as t] + [integrant.core :as ig] + [mockery.core :refer [with-mocks]]) + (:import + io.prometheus.client.Counter + io.prometheus.client.Counter$Child + io.prometheus.client.Histogram + io.prometheus.client.Histogram$Child + io.prometheus.client.Histogram$Child$Value + java.time.Duration + software.amazon.awssdk.core.metrics.CoreMetric + software.amazon.awssdk.metrics.MetricCollector)) + +(defn- make-metrics + [] + (ig/init-key :app.metrics/metrics + {:default (select-keys main/default-metrics + [:storage-s3-requests + :storage-s3-retries + :storage-s3-timing])})) + +(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- histogram-sum + [metrics id labels] + (let [collector (mtx/get-collector metrics id) + instance (::mdef/instance collector) + child (.labels ^Histogram instance (into-array String labels)) + value (.get ^Histogram$Child child)] + (.-sum ^Histogram$Child$Value value))) + +(defn- api-call + [entries] + (let [collector (MetricCollector/create "ApiCall")] + (doseq [[metric value] entries] + (.reportMetric ^MetricCollector collector metric value)) + (.collect ^MetricCollector collector))) + +(t/deftest publisher-records-operation-retries-and-duration + (let [metrics (make-metrics) + publisher (s3m/wrap-publisher metrics :default) + call (api-call [[CoreMetric/OPERATION_NAME "PutObject"] + [CoreMetric/API_CALL_SUCCESSFUL true] + [CoreMetric/RETRY_COUNT 2] + [CoreMetric/API_CALL_DURATION (Duration/ofMillis 12)]])] + (.publish publisher call) + (t/is (= 1.0 (counter-value metrics :storage-s3-requests ["PutObject" "default" "ok"]))) + (t/is (= 2.0 (counter-value metrics :storage-s3-retries ["PutObject" "default"]))) + (t/is (= 12.0 (histogram-sum metrics :storage-s3-timing ["PutObject" "default"]))))) + +(t/deftest publisher-ignores-invalid-collections + (let [metrics (make-metrics) + publisher (s3m/wrap-publisher metrics :default) + empty-call (.collect ^MetricCollector (MetricCollector/create "ApiCall"))] + (t/is (nil? (.publish publisher empty-call))) + (t/is (= 0.0 (counter-value metrics :storage-s3-requests ["PutObject" "default" "ok"]))) + (t/is (= 0.0 (counter-value metrics :storage-s3-requests ["unknown" "default" "ok"]))))) + +(t/deftest publisher-records-failed-calls + (let [metrics (make-metrics) + publisher (s3m/wrap-publisher metrics :default) + call (api-call [[CoreMetric/OPERATION_NAME "PutObject"] + [CoreMetric/API_CALL_SUCCESSFUL false] + [CoreMetric/RETRY_COUNT 0] + [CoreMetric/API_CALL_DURATION (Duration/ofMillis 7)]])] + (.publish publisher call) + (t/is (= 1.0 (counter-value metrics :storage-s3-requests ["PutObject" "default" "error"]))) + (t/is (= 0.0 (counter-value metrics :storage-s3-requests ["PutObject" "default" "ok"]))) + (t/is (= 7.0 (histogram-sum metrics :storage-s3-timing ["PutObject" "default"]))))) + +(t/deftest publisher-labels-custom-target + (let [metrics (make-metrics) + publisher (s3m/wrap-publisher metrics :eu-west) + call (api-call [[CoreMetric/OPERATION_NAME "GetObject"] + [CoreMetric/API_CALL_SUCCESSFUL true] + [CoreMetric/RETRY_COUNT 0] + [CoreMetric/API_CALL_DURATION (Duration/ofMillis 3)]])] + (.publish publisher call) + (t/is (= 1.0 (counter-value metrics :storage-s3-requests ["GetObject" "eu-west" "ok"]))) + (t/is (= 0.0 (counter-value metrics :storage-s3-requests ["GetObject" "default" "ok"]))))) + +(t/deftest publisher-survives-record-failure + (let [metrics (make-metrics) + publisher (s3m/wrap-publisher metrics :default) + call (api-call [[CoreMetric/OPERATION_NAME "PutObject"] + [CoreMetric/API_CALL_SUCCESSFUL true] + [CoreMetric/RETRY_COUNT 0] + [CoreMetric/API_CALL_DURATION (Duration/ofMillis 5)]])] + (with-mocks [_mock {:target 'app.metrics/run-collector! + :throw (ex-info "boom" {})}] + (t/is (nil? (.publish publisher call)))) + (t/is (= 0.0 (counter-value metrics :storage-s3-requests ["PutObject" "default" "ok"]))))) + +(t/deftest publisher-treats-missing-success-flag-as-error + ;; Documents the nil policy: a present operation without a success flag + ;; counts as an error so silent SDK changes surface on dashboards. + (let [metrics (make-metrics) + publisher (s3m/wrap-publisher metrics :default) + call (api-call [[CoreMetric/OPERATION_NAME "PutObject"]])] + (.publish publisher call) + (t/is (= 1.0 (counter-value metrics :storage-s3-requests ["PutObject" "default" "error"]))) + (t/is (= 0.0 (counter-value metrics :storage-s3-requests ["PutObject" "default" "ok"]))))) + +(t/deftest publisher-skips-retries-and-timing-when-absent + (let [metrics (make-metrics) + publisher (s3m/wrap-publisher metrics :default) + call (api-call [[CoreMetric/OPERATION_NAME "GetObject"] + [CoreMetric/API_CALL_SUCCESSFUL true] + [CoreMetric/RETRY_COUNT 0]])] + (.publish publisher call) + (t/is (= 1.0 (counter-value metrics :storage-s3-requests ["GetObject" "default" "ok"]))) + (t/is (= 0.0 (counter-value metrics :storage-s3-retries ["GetObject" "default"]))) + (t/is (= 0.0 (histogram-sum metrics :storage-s3-timing ["GetObject" "default"]))))) + +(t/deftest s3-backend-is-wired-with-optional-metrics + (t/is (= (ig/ref ::mtx/metrics) + (get-in main/system-config [:app.storage.s3/backend ::mtx/metrics])))) diff --git a/backend/test/backend_tests/storage_test.clj b/backend/test/backend_tests/storage_test.clj index 519c21dafb..54953620a0 100644 --- a/backend/test/backend_tests/storage_test.clj +++ b/backend/test/backend_tests/storage_test.clj @@ -97,7 +97,7 @@ :content-type "text/plain" :expired-at (ct/in-future {:seconds 1})})] (t/is (sto/object? object)) - (t/is (true? (sto/del-object! storage object))) + (t/is (true? (sto/del-object! storage (:id object)))) ;; retrieving the same object should be not nil because the ;; deletion is not immediate