Add log crate?

[michaelkirk]

What would folks think of starting to use the log crate in geo?

In general, I think high quality permanent logging is a worthwhile addition to the software, especially as more outside people use the crate and encounter errors.

But for me this was spurred by the work I'm doing in #515, some of which is pretty complicated. Consequently I've introduced some non-trivial logging while I work out the bugs. I think some of this would be valuable to keep around if future problems arise or changes made. Typically I just delete my print statements once I'm confident everything is working, but since this is a larger more complicated component, my preference would be to leave some of it in - mostly at debug! or lower log levels, so people won't typically see it.

The log crate is already used transitively by some of our features (postgres, proj_network) but it's not currently required by the default geo build.

The biggest shortcoming I see is that I know of only awkward ways to utilize it while developing locally and while testing, e.g. https://stackoverflow.com/questions/30177845/how-to-initialize-the-logger-for-integration-tests

[rmanoka]

I like this idea. Have felt the need to debug by printing messages while implementing a few algos in this crate.

On a related note, should we add Debug to the traits defining CoordinateType ? I found it very difficult to quickly add a few eprintln lines to print coordinates from within algorithms while debugging. I can't imagine scalar type that wouldn't support Debug.

[michaelkirk]

should we add Debug to the traits defining CoordinateType

I'm in favor of that. It's technically a geo-types breaking change though 😢.

[urschrei]

I'm in favor of that. It's technically a geo-types breaking change though 😢.

Bah. We should do both, though. Debugging is enough of a pain as it is.

[michaelkirk]

should we add Debug to the traits defining CoordinateType

Just in case I wasn't clear, I agree that we should do it.

I wonder if there's some slightly graceful way we could introduce this feature in a way that allows deprecation first.

Like introduce a new trait that implements Debug - maybe by way of #540?

edit: to clarify - #540 is about Coordinate, not CoordinateType, but I think a similar idea of deprecation and renaming is at play. And maybe it makes sense to do both at the same time to "rip off the bandaid".

[frewsxcv]

Created a new issue for the Debug conversation. +1 from me. #597

@michaelkirk Back to this issue, do you have an example of where logging in this crate would be helpful? I'm fine with this, just want to get an idea of how it'll get implemented.

[michaelkirk]

There's currently a few println!/eprintln! calls in the codebase. Those would be good candidates to instead be log macros.

I think typically I'm just adding println's and deleting it before committing. Which is fine. But clear understandable logging takes some effort, and if you revisit code later you have to re-implement that logging, so there can be tension between writing "good" logging and writing "quick" logging since you're always reimplementing it.

At the moment I'm specifically interested in keeping some of the logging around tha I've written while debugging the denim stuff. I'd prefer to keep in some lines like this at a debug! level so they can be turned on together.

geo/geo/src/algorithm/relate/relate_computer.rs

Line 317 in 10ce494

"after updated_intersection_matrix(isolated_edge: {:?}, label: {:?}): {:?}",

You can grep that commit for println to see some others. There's a sequence of mutating method calls and tracing values across them has been helpful for me.

As a side note, I usually turn to the env_logger backend to actually drive the log crate, because it lets me control which logs get printed based on log level at a per crate and per module level.

I can say RUST_LOG=info,geo=debug,geo::geomgraph=verbose cargo run to get verbose! logging for the geo::geomgraph module, slightly less noisy debug! level logging for the rest of the geo crate, and info! level logging for everything else.`

[michaelkirk]

added in #639