lambdaisland / glogi

A ClojureScript logging library based on goog.log
Mozilla Public License 2.0
119 stars 13 forks source link

Removing logging from production builds #26

Closed rome-user closed 1 year ago

rome-user commented 1 year ago

In release builds I am noticing trace-level and debug-level log messages that I do not want in the compiled JS file. Mostly to save space because my code has a lot of trace-level and debug-level logs for development purposes.

I have taken a quick look at the macros like trace and debug. They ultimately return forms invoking a private log-expr function. Would it be possible to offer an option to disable logging entirely? This could be done within the logging macros themselves, or introducing closure constant lambdaisland.glogi/enabled then wrapping the body of log-expr like so.

(when ^boolean lambdaisland.glogi/enabled
  (do stuff))

which either goes away entirely or becomes (do stuff) under advanced optimizations.

plexus commented 1 year ago

This can be configured on the Google closure level. In your cljs compiler config:

:closure-defines {goog.DEBUG false
                  goog.debug.LOGGING_ENABLED false}
rome-user commented 1 year ago

shadow-cljs by default will set goog.DEBUG to false. I have set goog.debug.LOGGING_ENABLED to false in release builds, but I still see all of my trace-level and debug-level log messages in the JS bundle. I am interested in getting rid of these maps and function calls from release builds

plexus commented 1 year ago

In that case I'm assuming the Google closure library introduced some breaking changes... Would be good to look into that so we can update our docs.

plexus commented 1 year ago

I don't see any relevant changes in google closure... this is still in there

goog.log.ENABLED = goog.define('goog.log.ENABLED', goog.debug.LOGGING_ENABLED);

and every logging call is guarded by if (goog.log.ENABLED) { ... }, which the google closure library will strip out if it's false and advanced optimizations are switched on. So there should be no logging code present in the final production build artifact.

We'll have to start with a minimal reproduction of the issue. @rome-user if you could help with that that would be really appreciated. Set up a small repo with only a dependency on glogi, configuration for a cljs build tool, and a single namespace with some logging calls, with instructions on how to compile and run.

rome-user commented 1 year ago

In my case, I am successfully getting DCE with the following modification to log-expr

(defn- log-expr [form level keyvals]
  (let [keyvals-map (apply array-map keyvals)
        formatter (::formatter keyvals-map 'identity)]
    `(when ~(with-meta 'goog.debug.LOGGING_ENABLED {:tag 'boolean})
       (log ~(::logger keyvals-map (str *ns*))
            ~level
            (~formatter ~(-> keyvals-map
                             (dissoc ::logger)
                             (assoc :line (:line (meta form)))))
            ~(:exception keyvals-map)))))

But with the original log-expr function, I am seeing a bunch of maps in my compiled JS file that correspond to inputs to macros like log/debug. I will attempt making a small repository soon to demonstrate this.

plexus commented 1 year ago

Are you seeing logs in your console/stdout or not?

plexus commented 1 year ago

A PR for further minimizing what we emit when logging is disabled is certainly welcome!

rome-user commented 1 year ago

Logs are not being printed. But in the compiled JS file I can see the strings that would be logged if I had e.g. installed the console logger. Basically, the data structures we use for logging do not get DCE'd unless I modify log-expr as above.

I am currently going to set up a small shadow-cljs project demonstrating this. Then I will upload the repo so you can take a look

rome-user commented 1 year ago

@plexus Sorry for taking a long time. Here is the repository https://github.com/rome-user/glogi-demo

plexus commented 1 year ago

Sorry for misunderstanding your issue. In that case the behavior you are seeing is to be expected with the current implementation. A patch to elide logging code when LOGGING_ENABLED is false would be more than welcome!