Add Caddy json log support

matomo-org / matomo-log-analytics

Import any kind of server logs in Matomo for powerful log analytics. Universal log file parsing and reporting.

https://matomo.org/log-analytics/

GNU General Public License v3.0

224 stars 118 forks source link

Add Caddy json log support #367

Closed simon04 closed 5 months ago

simon04 commented 6 months ago

Description:

Add Caddy json log support, https://caddyserver.com/docs/caddyfile/directives/log

Review

[ ] Functional review done
[ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
[ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
[ ] Security review done
[ ] Wording review done
[ ] Code review done
[ ] Tests were added if useful/possible
[ ] Reviewed for breaking changes
[ ] Developer changelog updated if needed
[ ] Documentation added if needed
[ ] Existing documentation updated if needed

michalkleiner commented 6 months ago

Hi @simon04, thank you for taking the time and opening the PR adding the Caddy log support. I'm not a Caddy user myself but I was wondering if the PR change could be done without adding the types? Is that required for it to work or could we do without the typings? It seems the most risky part of the change as no other format import uses that.

simon04 commented 5 months ago

Hi @michalkleiner, the types are defined in https://peps.python.org/pep-0484/ and are being used more and more in the Python ecosystem. Sure, the code works w/o types as well, but we'd loose some static analysis capabilities.

Since the type classes are wrapped in typing.TYPE_CHECKING, no harm should be done for production:

A special constant that is assumed to be True by 3rd party static type checkers. It is False at runtime. https://docs.python.org/3.12/library/typing.html#typing.TYPE_CHECKING

Please let me know how to proceed...

michalkleiner commented 5 months ago

@sgiehl what's your opinion here? I'd accept the PR without the types, but if you're happy with it as is then all good with me.

sgiehl commented 5 months ago

@simon04 Yes. Please remove the typing for now. We can consider adding that at a later step for the whole script if useful. Please also rebase your branch on the latest changes from 5.x-dev, hopefully this will trigger the tests to run for this PR.

simon04 commented 5 months ago

Hi @michalkleiner and @sgiehl, I've made the progress in static type checking undone. Feel free to merge now.

michalkleiner commented 5 months ago

Thank you for the contribution, @simon04!