Supporting log level for log-requests from configuration #20

lambdadbmal · 2023-02-14T08:59:21Z

Add new configuration feature to modify log-requests' log level.

You can modify requests log level in your config like ...

:duct.middleware.web/log-requests {:level :debug}

Also you can hide all requests log like ...

:duct.middleware.web/log-requests {:level :debug}
:duct.logger/timbre {:level :info}

weavejester

Thanks for the PR. This seems a reasonable change. I've reviewed the code and made some comments.

weavejester · 2023-02-15T15:32:56Z

src/duct/middleware/web.clj

+(defn- log-request
+  ([logger request]
+  (log-request logger request :info))
+  ([logger request level]
+  (logger/log logger level ::request (select-keys request request-log-keys))))


This is a private function, so it doesn't need two different arities. Also take care to ensure that lines are indented correctly.

Thank you for reviewing.
I delete older [logger request] arity. Now there is only `[logger request level]'.
I also reformatted my code with cljfmt.

weavejester · 2023-02-15T15:34:02Z

src/duct/middleware/web.clj

  (fn
    ([request]
-     (log-request logger request)
+     (log-request logger request (or level :info))


level is :info by default, so there's no need to check for nil at this point.

Thank you.
I removed (or ...) and change it to (log-request logger request level).

weavejester · 2023-02-15T15:37:03Z

src/duct/module/web.clj

+(defn- logging-config [config]
+  {:duct.middleware.web/log-requests (or (:duct.middleware.web/log-requests config) {})


I don't understand the purpose of these changes. Why not just let the configurations merge normally? Why add this exception?

Thank you.
That is my misunderstand of configuration.
Now I rollbacked original code.
And I added test code for changing log level via config in test/duct/module/web_test.clj.

weavejester · 2023-02-15T15:41:40Z

Also, can you change the commit message to:

Add support for changing log level of requests

weavejester · 2023-02-17T02:46:04Z

src/duct/middleware/web.clj

+(defn- log-request
+  ([logger request level]
+   (logger/log logger level ::request (select-keys request request-log-keys))))


Remove the unnecessary (). e.g.

(defn- log-request [logger level request] (logger/log logger :info ::request (select-keys request request-log-keys)))

weavejester · 2023-02-17T02:47:12Z

src/duct/middleware/web.clj

+(defmethod ig/init-key ::log-requests [_ {:keys [logger level]}]
+  #(wrap-log-requests % logger level))


Add a default for log levels here. e.g.

(defmethod ig/init-key ::log-requests [_ {:keys [logger level] :or {level :info}}] #(wrap-log-requests % logger level))

weavejester · 2023-02-17T02:49:00Z

src/duct/module/web.clj

@@ -34,7 +34,7 @@

 (def ^:private logging-config
  {:duct.middleware.web/log-requests {}
-   :duct.middleware.web/log-errors   {}
+   :duct.middleware.web/log-errors {}


Please ensure your diff doesn't include unnecessary spacing alterations.

weavejester · 2023-02-17T02:49:13Z

test/duct/module/web_test.clj

+      (is (= {:urlencoded true :keywordize false}
+             (:params (:duct.middleware.web/defaults config'))))))


Please ensure your diff doesn't include unnecessary spacing alterations.

weavejester · 2023-02-17T02:50:00Z

test/duct/middleware/web_test.clj

+      (let [logs    (atom [])
+            handler  (wrap-log-requests (constantly response) (->TestLogger logs) :trace)]


Ensure that the let clauses are correctly aligned, and the lines are 80 characters or under.

lambdadbmal · 2023-02-17T04:02:01Z

Thank you for reviewing again.
I fixed all of you mentioned.

weavejester · 2023-02-17T05:08:30Z

src/duct/middleware/web.clj

+(defn- log-request
+  [logger request level]
+  (logger/log logger level ::request (select-keys request request-log-keys)))


There's no need to put [logger request level] on a new line.

weavejester · 2023-02-17T05:12:05Z

test/duct/middleware/web_test.clj

+      (let [logs     (atom [])
+            handler  (wrap-log-requests (constantly response)
+                       (->TestLogger logs) :trace)]


Reformat according to style guidelines. i.e.

(let [logs (atom []) handler (wrap-log-requests (constantly response) (->TestLogger logs) :trace)]

weavejester · 2023-02-17T05:12:44Z

test/duct/module/web_test.clj

+                            {:duct.middleware.web/log-requests {:level :debug}})
+          config' (core/build-config config)]
+      (is (= {:level :debug}
+             (:duct.middleware.web/log-requests (:duct.middleware.web/defaults config'))))))


Ensure lines are within 80 characters.

lambdadbmal · 2023-02-17T08:21:42Z

Thank you.
I fixed codes as you mentioned.
I also squashed all of commits.

weavejester · 2023-02-17T16:29:18Z

test/duct/middleware/web_test.clj

+      (let [logs     (atom [])
+            handler  (wrap-log-requests (constantly response)
+                                        (->TestLogger logs)
+                                        :trace)]


As per my last comment, could you fix the spacing in the let clause? There are 2 spaces after handler instead of 1. Otherwise it looks fine.

weavejester

Hi, sorry for the delay. I've been busy working on fixing an issue in Ring. Now that I've come back to it, I've noticed that wrap-log-requests is a public function, and that we should probably use an option map instead, so we that don't have to change the API if we want to add any more functionality. Apologies for not thinking about that sooner.

weavejester · 2023-02-27T17:02:32Z

src/duct/middleware/web.clj

-     (handler request respond raise))))
+  ([handler logger]
+   (wrap-log-requests handler logger :info))
+  ([handler logger level]


I was thinking that to future-proof this API, it would make sense to make the third argument an options map. So instead of level, change it to {:keys [level] :or {level :info}}. Then in the ::log-requests method we can pass the options directly, minus the logger: (dissoc options :logger).

Thanks for you reply (and also contributing to Ring. We all clojurian thank a lot for your work).

I pushed new commit with your feedback.
I added third parameter to wrap-log-requests function as an option map.
And I revised test-wrap-log-requests to add a third parameter.
(We always call wrap-log-requests with three parameters. Is it correct?)

lambdadbmal · 2023-03-15T23:51:30Z

Hi, how's it going?
I would think you are still busy.
If you have a time to continue handling this PR, please let me know.
Take it easy.

weavejester

Sorry for forgetting about this PR! Feel free to poke me whenever I take too long. I've looked it over, and I think after the change I've outlined it should be good to merge. Thanks for your work on this.

weavejester · 2023-03-16T01:25:46Z

src/duct/middleware/web.clj


 (defn- log-error [logger ex]
  (logger/log logger :error ::handler-error ex))

 (defn wrap-log-requests
  "Log each request using the supplied logger. The logger must implement the
  duct.core.protocols/Logger protocol."
-  [handler logger]
+  [handler logger {:keys [level] :or {level :info}}]


Can you add in a 2-arity version with a blank map? i.e.

(defn wrap-log-requests "..." ([handler logger] (wrap-log-requests handler logger {})) ([handler logger {:keys [level] :or {level :info}}] ...))

Sure. I will do it.

weavejester · 2023-03-16T01:27:01Z

test/duct/middleware/web_test.clj

    (testing "asynchronous"
      (let [logs     (atom [])
            handler  (wrap-log-requests
                      (fn [_ respond _] (respond response))
-                      (->TestLogger logs))
+                      (->TestLogger logs) {})


This {} can be removed when you have a default options argument.

weavejester · 2023-03-16T05:14:22Z

Thanks again for your work on this. Sorry it took so long to merge.

lambdadbmal · 2023-03-16T05:24:22Z

Thank you for merging my PR.

lambdadbmal mentioned this pull request Feb 15, 2023

Support log-requests' log level configurable #21

Open

lambdadbmal force-pushed the feature/add-logging-config branch from 68dee99 to 2940593 Compare February 15, 2023 07:51

weavejester requested changes Feb 15, 2023

View reviewed changes

lambdadbmal requested a review from weavejester February 17, 2023 01:55

lambdadbmal force-pushed the feature/add-logging-config branch from 425282c to 288762c Compare February 17, 2023 02:44

weavejester requested changes Feb 17, 2023

View reviewed changes

lambdadbmal force-pushed the feature/add-logging-config branch from 943c43e to 2268e28 Compare February 17, 2023 04:01

lambdadbmal requested a review from weavejester February 17, 2023 04:03

weavejester requested changes Feb 17, 2023

View reviewed changes

lambdadbmal force-pushed the feature/add-logging-config branch from 2268e28 to f6b1524 Compare February 17, 2023 08:19

lambdadbmal requested a review from weavejester February 17, 2023 08:23

weavejester reviewed Feb 17, 2023

View reviewed changes

lambdadbmal force-pushed the feature/add-logging-config branch from f6b1524 to 79dc5a0 Compare February 18, 2023 01:31

lambdadbmal requested a review from weavejester February 18, 2023 01:33

weavejester requested changes Feb 27, 2023

View reviewed changes

lambdadbmal force-pushed the feature/add-logging-config branch from 79dc5a0 to dabd146 Compare March 1, 2023 00:58

lambdadbmal requested a review from weavejester March 1, 2023 01:03

weavejester requested changes Mar 16, 2023

View reviewed changes

Add support for changing log level of requests

fe9a616

lambdadbmal force-pushed the feature/add-logging-config branch from dabd146 to fe9a616 Compare March 16, 2023 04:33

lambdadbmal requested a review from weavejester March 16, 2023 04:34

weavejester merged commit a5b61eb into duct-framework:master Mar 16, 2023

lambdadbmal mentioned this pull request Mar 17, 2023

Add support for changing log level of SQL duct-framework/database.sql.hikaricp#13

Open

blackawa deleted the feature/add-logging-config branch August 18, 2023 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supporting log level for log-requests from configuration #20

Supporting log level for log-requests from configuration #20

lambdadbmal commented Feb 14, 2023 •

edited

Loading

weavejester left a comment

weavejester Feb 15, 2023

lambdadbmal Feb 17, 2023

weavejester Feb 15, 2023

lambdadbmal Feb 17, 2023

weavejester Feb 15, 2023

lambdadbmal Feb 17, 2023

weavejester commented Feb 15, 2023

weavejester Feb 17, 2023

weavejester Feb 17, 2023

weavejester Feb 17, 2023

weavejester Feb 17, 2023

weavejester Feb 17, 2023

lambdadbmal commented Feb 17, 2023

weavejester Feb 17, 2023

weavejester Feb 17, 2023

weavejester Feb 17, 2023

lambdadbmal commented Feb 17, 2023

weavejester Feb 17, 2023

weavejester left a comment

weavejester Feb 27, 2023

lambdadbmal Mar 1, 2023

lambdadbmal commented Mar 15, 2023

weavejester left a comment

weavejester Mar 16, 2023

lambdadbmal Mar 16, 2023

weavejester Mar 16, 2023

weavejester commented Mar 16, 2023

lambdadbmal commented Mar 16, 2023

		(defn- logging-config [config]
		{:duct.middleware.web/log-requests (or (:duct.middleware.web/log-requests config) {})

		(defmethod ig/init-key ::log-requests [_ {:keys [logger level]}]
		#(wrap-log-requests % logger level))

		(is (= {:urlencoded true :keywordize false}
		(:params (:duct.middleware.web/defaults config'))))))

		(let [logs (atom [])
		handler (wrap-log-requests (constantly response) (->TestLogger logs) :trace)]

Supporting log level for log-requests from configuration #20

Supporting log level for log-requests from configuration #20

Conversation

lambdadbmal commented Feb 14, 2023 • edited Loading

weavejester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weavejester commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdadbmal commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdadbmal commented Feb 17, 2023

Choose a reason for hiding this comment

weavejester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdadbmal commented Mar 15, 2023

weavejester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weavejester commented Mar 16, 2023

lambdadbmal commented Mar 16, 2023

lambdadbmal commented Feb 14, 2023 •

edited

Loading