Fixes #76

[piotrmaslanka]

Since this introduces some breaking changes, I'd invite @tintoy to express what should be left.

Right now everything can be configured using logging.config.dictConfig() including CLEF, override_root_logger and use_structured_logging

Also, this adds an option for ConsoleStructuredLogHandler to output to stderr. It's a config option.

I've tested it on my staging environment.

Also, I wonder, because right now by configuring one handler you can change the feature flags globally (for other loggers), shouldn't we move that to private properties of SeqLogHandlers? Because this behavior is not immediately obvious and somewhat astonishing.

Also, current implementation allows you to configure whether to use CLEF on the level of a ConsoleStructuredLogHandler, which is a code smell (and just plainly doesn't make sense, as a ConsoleStructuredLogHandler will never have to deal with CLEF), which amounts to, how one physicist put it as "spooky action at a distance".

I suggest we slowly shift towards per-handler configuration instead of global feature flags.

[tintoy]

Thanks! I’m hoping to give this my full attention sometime over the weekend.

[tintoy]

BTW, I agree with you about feature flags; that started off as something truly global but over time has started to include things that should be per-logger. Part of the problem is that our loggers are created by the logging library (which appears to only support a global configuration). But it’s been ages since I looked at it so if you know of a more-specific configuration mechanism then I’m happy for us to use that.

[piotrmaslanka]

I suggest the following solution:

• the things that should stay local (like using CLEF or not reporting failures) will be moved into the constructor of the SeqLogHandler
• the things that should stay global (like global properties) - they will still stay global

But I still have the following problems:

• overriding the logger can't be done via dictConfig(), so it has to stay as a separate method, preferably to be invoked by the user before they even touch logging - logging.setLoggerClass(StructuredLogger) has a nice ring to it
• either the behaviour of StructuredLogger will continue on global properties or we reimplement dictConfig() to allow people to configure their loggers. Right now, this is not possible, as they are allowed to take only 2 arguments - name and level, and I would like to keep it that way, lest we unleash hell. Imagine - every logger variable in each module could behave in a different way. This is pure madness.

Or:

• We can define our own dictConfig() that will do the job, pop some parameters, and hand over the rest to Python's dictConfig() that will take a bunch of keys like use_structured_logging or override_logger. I think this is the best solution, overall. fileConfig()'s just not worth that much since they changed the syntax, and I suggest we entirely abandon it.

What say you, @tintoy ?

[tintoy]

Yeah, I think part of the problem is that python's logging module is essentially global

We can define our own dictConfig() that will do the job

This was what configure_from_dict was meant to handle but it has gradually mutated over time 🙂

logging.setLoggerClass(StructuredLogger) has a nice ring to it

I'm hoping we can provide a more intuitive API to do this (just a function like use_seq_structured_logging() or similar) because, while consumers need to set the logger class, it's not all that obvious why they need to set the logger class.

I would like to keep it that way, lest we unleash hell

No argument here 😂

If we had to, I guess we could introduce a logger-name-or-regex approach to individual logger instance configuration, but again, it might start to feel a bit like reinventing the logging module 🤪

We can define our own dictConfig() that will do the job, pop some parameters, and hand over the rest to Python's dictConfig() that will take a bunch of keys like use_structured_logging or override_logger. I think this is the best solution, overall. fileConfig()'s just not worth that much since they changed the syntax, and I suggest we entirely abandon it.

Yes - this feels much cleaner to me, too. I'm not too fussed about removing functionality that doesn't make sense as long as we sketch out a migration path for existing consumers in the docs ("migrating from v0.x" could include a workaround for the fact that fileConfig() has changed, such as "load YAML from file into dict, and call seqlog's configure_from_dict)...

[tintoy]

I like the if_not_configured, too. Much more deterministic!

[tintoy]

Some minor suggestions in the PR comments, but either way this looks good to me 🙂

[piotrmaslanka]

@tintoy I've decided to practically completely refactor the project and move all of the configuration into the dictionary/YAML.

I'm currently testing it on my staging environment for my production (so far everything works), and it seems that you can now configure it with a single command.

I'm re-requesting a review.

[tintoy]

Cheers - will take a look at this later today 🙂

[tintoy]

Will take a closer look on my computer but, at first glance (hooray for Code Spaces - even on an iPhone 😂), this looks great!

[tintoy]

Had a look on a full-size monitor and, yep, this looks great!

[tintoy]

Sorry, I don't know how I lost track of this PR!

I'll get it merged tomorrow morning 🙂

[tintoy]

(thanks for all the effort you put into it!)

[tintoy]

@piotrmaslanka - I'd like to merge this, but there are conflicts I can't resolve from my end; if you have time, would you mind applying this commit on your end to resolve them?

50ba855

(sorry again for losing track of your contribution!)

[tintoy]

Closing in favour of PR #93 (in which these changes have been rebased against the current master branch). Work is being tracked by #92.