From 3b44f9330845d1ab89c8c5b883ea139377cb5a23 Mon Sep 17 00:00:00 2001 From: Arttu Kaipiainen Date: Fri, 20 Oct 2023 14:58:26 +0300 Subject: [PATCH 1/3] Fix transaction handling in async middleware --- src/clojure_elastic_apm/ring.clj | 46 ++++++++++++++------- test/clojure_elastic_apm/ring_test.clj | 56 ++++++++++++++++++-------- 2 files changed, 71 insertions(+), 31 deletions(-) diff --git a/src/clojure_elastic_apm/ring.clj b/src/clojure_elastic_apm/ring.clj index ab43d74..0ed0a24 100644 --- a/src/clojure_elastic_apm/ring.clj +++ b/src/clojure_elastic_apm/ring.clj @@ -1,6 +1,6 @@ (ns clojure-elastic-apm.ring (:require - [clojure-elastic-apm.core :refer [type-request with-apm-transaction]])) + [clojure-elastic-apm.core :as apm :refer [type-request with-apm-transaction]])) (defn match-uri [pattern uri] (let [pattern-segs (clojure.string/split pattern #"/") @@ -39,13 +39,21 @@ response)))) ([{:keys [request-method uri headers] :as request} respond raise] (let [tx-name (str (.toUpperCase (name request-method)) " " uri) - traceparent (get-in headers ["traceparent"])] - (with-apm-transaction [tx {:name tx-name :type type-request :traceparent traceparent}] - (let [req (assoc request :clojure-elastic-apm/transaction tx)] - (handler req (fn [{:keys [status] :as response}] - (when status - (.setResult tx (str "HTTP " status))) - (respond response)) raise))))))) + traceparent (get-in headers ["traceparent"]) + tx (apm/start-transaction {:name tx-name :type type-request :traceparent traceparent}) + req (assoc request :clojure-elastic-apm/transaction tx)] + (with-open [_ (apm/activate tx)] + (handler req + (fn [{:keys [status] :as response}] + (when status + (.setResult tx (str "HTTP " status))) + (apm/end tx) + (respond response)) + (fn [err] + (when (instance? Exception err) + (apm/capture-exception tx err)) + (apm/end tx) + (raise err)))))))) ([handler patterns] (fn ([{:keys [request-method uri headers] :as request}] @@ -65,10 +73,18 @@ tx-name (str (.toUpperCase (name request-method)) " " matched) traceparent (get-in headers ["traceparent"])] (if matched - (with-apm-transaction [tx {:name tx-name :type type-request :traceparent traceparent}] - (let [req (assoc request :clojure-elastic-apm/transaction tx)] - (handler req (fn [{:keys [status] :as response}] - (when status - (.setResult tx (str "HTTP " status))) - (respond response)) raise))) - (handler respond raise))))))) + (let [tx (apm/start-transaction {:name tx-name :type type-request :traceparent traceparent}) + req (assoc request :clojure-elastic-apm/transaction tx)] + (with-open [_ (apm/activate tx)] + (handler req + (fn [{:keys [status] :as response}] + (when status + (.setResult tx (str "HTTP " status))) + (apm/end tx) + (respond response)) + (fn [err] + (when (instance? Exception err) + (apm/capture-exception tx err)) + (apm/end tx) + (raise err))))) + (handler request respond raise))))))) diff --git a/test/clojure_elastic_apm/ring_test.clj b/test/clojure_elastic_apm/ring_test.clj index 16358bc..0f80365 100644 --- a/test/clojure_elastic_apm/ring_test.clj +++ b/test/clojure_elastic_apm/ring_test.clj @@ -41,19 +41,22 @@ (is (nil? (:parent tx-details)))))) (deftest wrap-apm-transaction-with-pattern-async-test - (let [transaction-id (atom nil) + (let [transaction-id (promise) request {:request-method :get, :uri "/foo/bar"} response {:status 200 :body "Ok."} + response-promise (promise) respond (fn [r] - (is (= (:status r) 200)) - r) + (deliver response-promise r)) handler (fn [request respond _] - (is (not= "" (.getId (apm/current-apm-transaction))) "transaction should've been activated for the duration of the request") - (is (= (.getId (:clojure-elastic-apm/transaction request)) (.getId (apm/current-apm-transaction))) "transaction should've been activated for the duration of the request") - (reset! transaction-id (.getId (:clojure-elastic-apm/transaction request))) - (respond response)) + (future + (with-open [_ (apm/activate (:clojure-elastic-apm/transaction request))] + (is (not= "" (.getId (apm/current-apm-transaction))) "transaction should've been activated for the duration of the request") + (is (= (.getId (:clojure-elastic-apm/transaction request)) (.getId (apm/current-apm-transaction))) "transaction should've been activated for the duration of the request") + (deliver transaction-id (.getId (:clojure-elastic-apm/transaction request))) + (respond response)))) wrapped-handler (apm-ring/wrap-apm-transaction handler ["/*/*"])] - (is (= (wrapped-handler request respond nil) response)) + (wrapped-handler request respond nil) + (is (= response (deref response-promise 1000 ::timeout))) (let [tx-details (es-find-first-document (str "(processor.event:transaction%20AND%20transaction.id:" @transaction-id ")"))] (is (= apm/type-request (get-in tx-details [:transaction :type]))) (is (= "HTTP 200" (get-in tx-details [:transaction :result]))) @@ -61,25 +64,46 @@ (is (nil? (:parent tx-details)))))) (deftest wrap-apm-transaction-async-test - (let [transaction-id (atom nil) + (let [transaction-id (promise) + response-promise (promise) request {:request-method :get, :uri "/foo/bar"} response {:status 200 :body "Ok."} respond (fn [r] - (is (= (:status r) 200)) - r) + (deliver response-promise r)) handler (fn [request respond _] - (is (not= "" (.getId (apm/current-apm-transaction))) "transaction should've been activated for the duration of the request") - (is (= (.getId (:clojure-elastic-apm/transaction request)) (.getId (apm/current-apm-transaction))) "transaction should've been activated for the duration of the request") - (reset! transaction-id (.getId (:clojure-elastic-apm/transaction request))) - (respond response)) + (future + (with-open [_ (apm/activate (:clojure-elastic-apm/transaction request))] + (is (not= "" (.getId (apm/current-apm-transaction))) "transaction should've been activated for the duration of the request") + (is (= (.getId (:clojure-elastic-apm/transaction request)) (.getId (apm/current-apm-transaction))) "transaction should've been activated for the duration of the request") + (deliver transaction-id (.getId (:clojure-elastic-apm/transaction request))) + (respond response)))) wrapped-handler (apm-ring/wrap-apm-transaction handler)] - (is (= (wrapped-handler request respond nil) response)) + (wrapped-handler request respond nil) + (is (= response (deref response-promise 1000 ::timeout))) + (is (not= "" @transaction-id)) (let [tx-details (es-find-first-document (str "(processor.event:transaction%20AND%20transaction.id:" @transaction-id ")"))] (is (= apm/type-request (get-in tx-details [:transaction :type]))) (is (= "HTTP 200" (get-in tx-details [:transaction :result]))) (is (= "GET /foo/bar" (get-in tx-details [:transaction :name]))) (is (nil? (:parent tx-details)))))) +(deftest wrap-apm-transaction-async-exception-test + (let [transaction-id (promise) + request {:request-method :get, :uri "/foo/bar"} + raise (fn [_]) + handler (fn [request _ raise] + (future + (with-open [_ (apm/activate (:clojure-elastic-apm/transaction request))] + (deliver transaction-id (.getId (:clojure-elastic-apm/transaction request))) + (raise (ex-info "Error in handler" {}))))) + wrapped-handler (apm-ring/wrap-apm-transaction handler)] + (wrapped-handler request nil raise) + (is (not= "" @transaction-id)) + (let [tx-details (es-find-first-document (str "(processor.event:error%20AND%20transaction.id:" @transaction-id ")"))] + (is (= apm/type-request (get-in tx-details [:transaction :type]))) + (is (= "clojure.lang.ExceptionInfo" (get-in tx-details [:error :exception 0 :type]))) + (is (= "Error in handler" (get-in tx-details [:error :exception 0 :message])))))) + (deftest wrap-apm-remote-transaction-test (let [transaction-id (atom nil) parent-id "3574dfeefa12d57e" From 6f88b0415217718f8dd5aaf411527654f8d79405 Mon Sep 17 00:00:00 2001 From: Arttu Kaipiainen Date: Mon, 23 Oct 2023 18:28:28 +0300 Subject: [PATCH 2/3] Refactor middleware: combine middlewares with or without patterns --- src/clojure_elastic_apm/ring.clj | 38 ++++++-------------------------- 1 file changed, 7 insertions(+), 31 deletions(-) diff --git a/src/clojure_elastic_apm/ring.clj b/src/clojure_elastic_apm/ring.clj index 0ed0a24..f1fbe4e 100644 --- a/src/clojure_elastic_apm/ring.clj +++ b/src/clojure_elastic_apm/ring.clj @@ -21,39 +21,16 @@ matched?))))) (defn match-patterns [patterns uri] - (->> patterns - (map #(match-uri % uri)) - (drop-while false?) - (first))) + (if patterns + (->> patterns + (map #(match-uri % uri)) + (drop-while false?) + (first)) + uri)) (defn wrap-apm-transaction ([handler] - (fn - ([{:keys [request-method uri headers] :as request}] - (let [tx-name (str (.toUpperCase (name request-method)) " " uri) - traceparent (get-in headers ["traceparent"])] - (with-apm-transaction [tx {:name tx-name :type type-request :traceparent traceparent}] - (let [{:keys [status] :as response} (handler (assoc request :clojure-elastic-apm/transaction tx))] - (when status - (.setResult tx (str "HTTP " status))) - response)))) - ([{:keys [request-method uri headers] :as request} respond raise] - (let [tx-name (str (.toUpperCase (name request-method)) " " uri) - traceparent (get-in headers ["traceparent"]) - tx (apm/start-transaction {:name tx-name :type type-request :traceparent traceparent}) - req (assoc request :clojure-elastic-apm/transaction tx)] - (with-open [_ (apm/activate tx)] - (handler req - (fn [{:keys [status] :as response}] - (when status - (.setResult tx (str "HTTP " status))) - (apm/end tx) - (respond response)) - (fn [err] - (when (instance? Exception err) - (apm/capture-exception tx err)) - (apm/end tx) - (raise err)))))))) + (wrap-apm-transaction handler nil)) ([handler patterns] (fn ([{:keys [request-method uri headers] :as request}] @@ -68,7 +45,6 @@ response)) (handler request)))) ([{:keys [request-method uri headers] :as request} respond raise] - (let [matched (match-patterns patterns uri) tx-name (str (.toUpperCase (name request-method)) " " matched) traceparent (get-in headers ["traceparent"])] From b5ce294c608b2fd5b833c5cd0fddbd7b0a398dba Mon Sep 17 00:00:00 2001 From: Ari Paasonen Date: Thu, 26 Oct 2023 10:16:41 +0300 Subject: [PATCH 3/3] Bump the version from 0.9.0 to 0.10.0 --- project.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project.clj b/project.clj index c8944e1..775c21a 100644 --- a/project.clj +++ b/project.clj @@ -1,4 +1,4 @@ -(defproject clojure-elastic-apm "0.9.0" +(defproject clojure-elastic-apm "0.10.0" :description "Clojure wrapper for Elastic APM Java Agent" :url "https://github.com/Yleisradio/clojure-elastic-apm" :license {:name "Eclipse Public License"