search

metosin / malli

High-performance data-driven data specification library for Clojure/Script.
Eclipse Public License 2.0
1.46k stars 208 forks source link

Make `instrument!` less verbose #842

Closed gpind closed 1 year ago

gpind commented 1 year ago

Addresses #689 by making -strument! return the affected vars instead of printing them, and then makes malli.dev print the count of affected vars when it calls instrument/unstrument.

ingesolvoll commented 1 year ago

This would be great! Could you also find some place where it makes sense to document this?

gpind commented 1 year ago

@ikitommi any objections to this?

ikitommi commented 1 year ago

Thanks for the PR! I think reducing printing noice is really needed. But just one :debug is bit too simple imo. For large codebases, a full re-instrument takes some time, would be nice go get stats on the time taken too + how many fns were instrumented. We could add incremental instrumentation to make it faster if perf becomes a bottleneck.

Could we have a extension mechanism here, that would allow supporting these both cases:

  1. collecting and printing individual instrumentations
  2. handling the whole instrumentation process

;; custom code to print per var
... instrumented xyz, took 12ms

;; custom code to print per whole thing
instrumented 32 vars, took 400ms

... I think a good default would be to print just the overall stats. Less noice, but still info that the process is running + numbers if that gets slow.

WDYT?

gpind commented 1 year ago

I'd rather make instrument! return information about what it instrumented, maybe just as a collection of vars or symbols? I can work on that, if you like the idea.

As for timing: the caller can easily time the call to instrument!, and finer-grained timing seems like a job for a profiler.

ikitommi commented 1 year ago

Returning data from instrument! seems like a good idea. In malli.dev, maybe default to printing just one line about the stats. If you feel it's still too much noice, the instrumentation reporting function could be configurable so one van remove it or do more reporting.

camsaul commented 1 year ago

Can we get this in and then do a separate follow-on to return those stats? I have a namespace with about 100 things getting instrumented and reloading it makes my REPL borderline unusable because of all the noise

gpind commented 1 year ago

@ikitommi I've implemented what we discussed; could you take another look?

ikitommi commented 1 year ago

Sorry for taking so long, looks good IMO. Thanks!