tolitius / cprop

likes properties, environments, configs, profiles..
Eclipse Public License 1.0
352 stars 25 forks source link

Don't use println for status reporting #13

Open pmonks opened 8 years ago

pmonks commented 8 years ago

In a couple of places cprop uses (println) for status logging, but it would be better if it used a logging framework (e.g. tools.logging).

Amongst other concerns of unconfigurable output to stdout, this messes with logging within Docker containers.

tolitius commented 8 years ago

Can you describe the concerns? how does it mess with a docker container?

The reason for printlns is mostly trace like debugging, and my inclination to keep no dependencies in the libraries. For example someone would like to use timbre or another logging library they should be able to.

But it does not mean I disagree with you, just want to start the dialog and see if we can find a better solution

pmonks commented 8 years ago

One standard way of logging in Docker is to write all logging output to stdout. While this is configurable when a logging library is in use (both in terms of what gets emitted and how it's formatted), (println) is completely unconfigurable - there is no way to shut it off* or control the formatting of the resulting output. Having unstructured output (from (println)) mixed in with structured output (from a logging library) in the log stream is problematic as it makes processing of that stream more difficult.

That said, I understand your reluctance to add 3rd party dependencies. Perhaps another solution would be to drop the use of (println) altogether? Those messages don't seem to be especially important.

* without using side-effecty tricks like rebinding *out*, which may have unforeseen consequences.

crimeminister commented 7 years ago

I'm using cprop and Pedestal together at the moment, so being able to ask cprop to log using io.pedestal.log would be cool.

tolitius commented 7 years ago

since println is a valid concern, I muted all of it.

Debugging can be unmuted with "export DEBUG=y", updated docs to cover that.

$ boot test

Testing cprop.core
Testing cprop.source
Testing cprop.tools
Testing cprop.core
Testing cprop.source
Testing cprop.test.core
Testing cprop.tools

Ran 12 tests containing 27 assertions.
0 failures, 0 errors.
$ export DEBUG=y

$ boot test

Testing cprop.core
Testing cprop.source
Testing cprop.tools
Testing cprop.core
Testing cprop.source

Testing cprop.test.core
(!) read config from resource: "empty.edn", but it is empty
(!) read config from file: "dev-resources/empty.edn", but it is empty
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
(!) read config from resource: "empty.edn", but it is empty
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "fill-me-in.edn"
read config from file: "dev-resources/fill-me-in.edn"
substituting [:io :http :pool :socket-timeout] with an ENV/System property specific value
substituting [:datomic :url] with an ENV/System property specific value
substituting [:aws :access-key] with an ENV/System property specific value
read config from resource: "config.edn"
read config from file: "dev-resources/fill-me-in.edn"
substituting [:datomic :url] with an ENV/System property specific value
substituting [:aws :access-key] with an ENV/System property specific value
substituting [:aws :secret-key] with an ENV/System property specific value
substituting [:aws :region] with an ENV/System property specific value
substituting [:io :http :pool :conn-timeout] with an ENV/System property specific value
substituting [:io :http :pool :max-per-route] with an ENV/System property specific value
substituting [:other-things] with an ENV/System property specific value
read config from stream: "dev-resources/config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/fill-me-in.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "config.edn"
read config from file: "dev-resources/config.edn"
read config from resource: "fill-me-in.edn"
read config from file: "dev-resources/fill-me-in.edn"
substituting [:io :http :pool :socket-timeout] with an ENV/System property specific value
substituting [:io :http :pool :max-total] with an ENV/System property specific value
substituting [:io :http :pool :conn-timeout] with an ENV/System property specific value
substituting [:datomic :url] with an ENV/System property specific value
substituting [:aws :access-key] with an ENV/System property specific value

Testing cprop.tools

Ran 12 tests containing 27 assertions.
0 failures, 0 errors.

@crimeminister I agree, it would be :) but I can't convince myself to include a particular logging dependency or to thread a custom logger across the functions just to say which config files were read.