From 2295d085d39e0733814060b2592d8fc8c9027271 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 27 Nov 2023 09:50:47 +0100 Subject: [PATCH] :zap: Improve performance on error formating and reporting --- backend/src/app/http/errors.clj | 24 ++++------------ backend/src/app/loggers/database.clj | 16 +++++------ backend/test/backend_tests/helpers.clj | 4 +-- common/deps.edn | 4 +++ common/src/app/common/exceptions.cljc | 35 ++++++++++++------------ common/src/app/common/files/builder.cljc | 2 +- common/src/app/common/pprint.cljc | 20 +++++++------- common/src/app/common/schema.cljc | 13 ++++----- frontend/src/app/main/errors.cljs | 7 ++--- 9 files changed, 56 insertions(+), 69 deletions(-) diff --git a/backend/src/app/http/errors.clj b/backend/src/app/http/errors.clj index 5bb14cc37..93c67845b 100644 --- a/backend/src/app/http/errors.clj +++ b/backend/src/app/http/errors.clj @@ -9,7 +9,7 @@ (:require [app.common.exceptions :as ex] [app.common.logging :as l] - [app.common.schema :as sm] + [app.common.schema :as-alias sm] [app.config :as cf] [app.http :as-alias http] [app.http.access-token :as-alias actoken] @@ -79,29 +79,15 @@ [err request parent-cause] (let [{:keys [code] :as data} (ex-data err)] (cond - (= code :spec-validation) + (or (= code :spec-validation) + (= code :params-validation) + (= code :data-validation)) (let [explain (ex/explain data)] {::rres/status 400 ::rres/body (-> data - (dissoc ::s/problems ::s/value ::s/spec) + (dissoc ::s/problems ::s/value ::s/spec ::sm/explain) (cond-> explain (assoc :explain explain)))}) - (= code :params-validation) - (let [explain (::sm/explain data) - explain (sm/humanize-data explain)] - {::rres/status 400 - ::rres/body (-> data - (dissoc ::sm/explain) - (assoc :explain explain))}) - - (= code :data-validation) - (let [explain (::sm/explain data) - explain (sm/humanize-data explain)] - {::rres/status 400 - ::rres/body (-> data - (dissoc ::sm/explain) - (assoc :explain explain))}) - (= code :request-body-too-large) {::rres/status 413 ::rres/body data} diff --git a/backend/src/app/loggers/database.clj b/backend/src/app/loggers/database.clj index 77694da27..e2892d13f 100644 --- a/backend/src/app/loggers/database.clj +++ b/backend/src/app/loggers/database.clj @@ -56,22 +56,22 @@ (dissoc :request/params :value :params :data))] (merge {:context (-> (into (sorted-map) ctx) - (pp/pprint-str :width 200 :length 50 :level 10)) - :props (pp/pprint-str props :width 200 :length 50) + (pp/pprint-str :length 50)) + :props (pp/pprint-str props :length 50) :hint (or (ex-message cause) @message) :trace (or (::trace record) (ex/format-throwable cause :data? false :explain? false :header? false :summary? false))} (when-let [params (or (:request/params context) (:params context))] - {:params (pp/pprint-str params :width 200 :length 50 :level 10)}) + {:params (pp/pprint-str params :length 30 :level 12)}) (when-let [value (:value context)] - {:value (pp/pprint-str value :width 200 :length 50 :level 10)}) + {:value (pp/pprint-str value :length 30 :level 12)}) (when-let [data (some-> data (dissoc ::s/problems ::s/value ::s/spec ::sm/explain :hint))] - {:data (pp/pprint-str data :width 200)}) + {:data (pp/pprint-str data :length 30 :level 12)}) - (when-let [explain (ex/explain data {:level 8 :length 20})] + (when-let [explain (ex/explain data :length 30 :level 12)] {:explain explain}))))) (defn error-record? @@ -96,11 +96,11 @@ (defmethod ig/init-key ::reporter [_ cfg] - (let [input (sp/chan :buf (sp/sliding-buffer 32) + (let [input (sp/chan :buf (sp/sliding-buffer 64) :xf (filter error-record?))] (add-watch l/log-record ::reporter #(sp/put! input %4)) - (px/thread {:name "penpot/database-reporter" :virtual true} + (px/thread {:name "penpot/database-reporter"} (l/info :hint "initializing database error persistence") (try (loop [] diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 6523477db..7273ff7dd 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -429,11 +429,11 @@ (= :params-validation (:code data)) (app.common.pprint/pprint - (sm/humanize-data (::sm/explain data))) + (sm/humanize-explain (::sm/explain data))) (= :data-validation (:code data)) (app.common.pprint/pprint - (sm/humanize-data (::sm/explain data))) + (sm/humanize-explain (::sm/explain data))) (= :service-error (:type data)) (print-error! (.getCause ^Throwable error)) diff --git a/common/deps.edn b/common/deps.edn index 41f1ced96..29aa6e1d8 100644 --- a/common/deps.edn +++ b/common/deps.edn @@ -48,6 +48,10 @@ ;; exception printing fipp/fipp {:mvn/version "0.6.26"} + io.github.eerohele/pp + {:git/tag "2023-11-25.47" + :git/sha "15d572c"} + io.aviso/pretty {:mvn/version "1.4.4"} environ/environ {:mvn/version "1.2.0"}} :paths ["src" "vendor" "target/classes"] diff --git a/common/src/app/common/exceptions.cljc b/common/src/app/common/exceptions.cljc index b68a77d1a..a5b9e47c6 100644 --- a/common/src/app/common/exceptions.cljc +++ b/common/src/app/common/exceptions.cljc @@ -65,23 +65,22 @@ (instance? RuntimeException v))) (defn explain - ([data] (explain data nil)) - ([data opts] - (cond - ;; NOTE: a special case for spec validation errors on integrant - (and (= (:reason data) :integrant.core/build-failed-spec) - (contains? data :explain)) - (explain (:explain data) opts) + [data & {:as opts}] + (cond + ;; NOTE: a special case for spec validation errors on integrant + (and (= (:reason data) :integrant.core/build-failed-spec) + (contains? data :explain)) + (explain (:explain data) opts) - (and (contains? data ::s/problems) - (contains? data ::s/value) - (contains? data ::s/spec)) - (binding [s/*explain-out* expound/printer] - (with-out-str - (s/explain-out (update data ::s/problems #(take (:length opts 10) %))))) + (and (contains? data ::s/problems) + (contains? data ::s/value) + (contains? data ::s/spec)) + (binding [s/*explain-out* expound/printer] + (with-out-str + (s/explain-out (update data ::s/problems #(take (:length opts 10) %))))) - (contains? data ::sm/explain) - (sm/humanize-data (::sm/explain data) opts)))) + (contains? data ::sm/explain) + (sm/humanize-explain (::sm/explain data) opts))) #?(:clj (defn format-throwable @@ -92,8 +91,8 @@ data? true explain? true chain? true - data-length 10 - data-level 4}}] + data-length 8 + data-level 5}}] (letfn [(print-trace-element [^StackTraceElement e] (let [class (.getClassName e) @@ -115,7 +114,7 @@ (print-data [data] (when (seq data) (print " dt: ") - (let [[line & lines] (str/lines (pp/pprint-str data :level data-level :length data-length ))] + (let [[line & lines] (str/lines (pp/pprint-str data :level data-level :length data-length))] (print line) (newline) (doseq [line lines] diff --git a/common/src/app/common/files/builder.cljc b/common/src/app/common/files/builder.cljc index d999a05fd..46c1e9131 100644 --- a/common/src/app/common/files/builder.cljc +++ b/common/src/app/common/files/builder.cljc @@ -50,7 +50,7 @@ (when-not valid? (let [explain (sm/explain ::ch/change change)] - (pp/pprint (sm/humanize-data explain)) + (pp/pprint (sm/humanize-explain explain)) (when fail-on-spec? (ex/raise :type :assertion :code :data-validation diff --git a/common/src/app/common/pprint.cljc b/common/src/app/common/pprint.cljc index c17bc5473..66925e7ef 100644 --- a/common/src/app/common/pprint.cljc +++ b/common/src/app/common/pprint.cljc @@ -7,16 +7,16 @@ (ns app.common.pprint (:refer-clojure :exclude [prn]) (:require - [fipp.edn :as fpp])) - -(defn pprint-str - [expr & {:keys [width level length] - :or {width 110 level 8 length 25}}] - (binding [*print-level* level - *print-length* length] - (with-out-str - (fpp/pprint expr {:width width})))) + [me.flowthing.pp :as pp])) (defn pprint + [expr & {:keys [width level length] + :or {width 120 level 8 length 25}}] + (binding [*print-level* level + *print-length* length] + (pp/pprint expr {:max-width width}))) + +(defn pprint-str [expr & {:as opts}] - (println (pprint-str expr opts))) + (with-out-str + (pprint expr opts))) diff --git a/common/src/app/common/schema.cljc b/common/src/app/common/schema.cljc index 1f939f15e..f1a86f7c2 100644 --- a/common/src/app/common/schema.cljc +++ b/common/src/app/common/schema.cljc @@ -152,19 +152,18 @@ (let [vfn (delay (decoder (if (delay? s) (deref s) s) transformer))] (fn [v] (@vfn v)))) -(defn humanize-data +(defn humanize-explain [{:keys [schema errors value]} & {:keys [length level]}] (let [errors (mapv #(update % :schema form) errors)] (with-out-str (println "Schema: ") - (println (pp/pprint-str (form schema) {:level (d/nilv level 10) - :length (d/nilv length 10)})) + (println (pp/pprint-str (form schema) {:width 100 :level 15 :length 20})) (println "Errors:") - (println (pp/pprint-str errors {:level (d/nilv level 10) - :length (d/nilv length 10)})) + (println (pp/pprint-str errors {:width 100 :level 15 :length 20})) (println "Value:") - (println (pp/pprint-str value {:level (d/nilv level 5) - :length (d/nilv length 10)}))))) + (println (pp/pprint-str value {:width 160 + :level (d/nilv level 8) + :length (d/nilv length 12)}))))) (defn pretty-explain [s d] diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index c6a25644a..f1c85b9f2 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -9,7 +9,7 @@ (:require [app.common.exceptions :as ex] [app.common.pprint :as pp] - [app.common.schema :as sm] + [app.common.schema :as-alias sm] [app.main.data.messages :as msg] [app.main.data.modal :as modal] [app.main.data.users :as du] @@ -33,9 +33,8 @@ (defn- print-explain! [data] - (when-let [explain (::sm/explain data)] - (js/console.log (sm/humanize-data explain))) - (when-let [explain (:explain data)] + (when-let [explain (or (ex/explain data) + (:explain data))] (js/console.log explain))) (defn- print-trace!