LGTM, looks great and will be beneficial for some low-level libraries and ops for sure.
This is a nice approach and Iām in favor of the separate ādisableā traits per specific log level rather than various combinations of āup toā etc.
A big +1, will be a nice win for apps that statically know their max log level.
I think the current approach is using the MaxLogLevel⦠syntax, so itās āup toā, rather than individual log levels.
I actually prefer the proposed approach of āup toā traits, as it matches how I imagine most people will want to use the feature, and it matches what Rust is doing as well.
If I understand correctly, this would also allow me to build my libraries into an SDK (binary), that has logging basically removed. Otherwise, clients could always just bootstrap the LoggingSystem so they get all my internal debug logging.
Whoops, seems we changed this since I reviewed this earlier and I didnāt realize⦠Thanks for pointing it out.
Iām not a huge fan of this as we lost granularity⦠but itās not something Iāll stand on my head arguing for. Thank you for linking to the rust case, it seems thatās working out fine there then.
Either way Iām supportive of the general mechanism anyway, so +1 remains
+1 on the proposal. I think this is a very needed addition to swift-log and enables library authors to add more logging statement while also enabling application authors to remove the runtime overhead for those. To me this proposal shows that our ecosystem is maturing since this is a high performance operational requirement.
I also think the proposal took the right approach by choosing the MaxLogLevel approach instead of disabling individual log levels.
I like this but āMax Log Levelā sounds weird. Intuitively it feels like ācriticalā should be a higher log level than āinfoā. āMaxLogLevelNoneā in particular feels counterintuitive (I would read this as no maximum log level, ie all logs are compiled in)
I totally feel the same. I always get confused which level is āhigherā. But I am fine with whatever is proposed, as it is a pre-existing condition, and I can always just finally get it into my head
Iām not really opposed to adding this as proposed, but there is one thought I have, which is related to what @FranzBusch wrote above:
While this is true when looking at a single library, what happens when many libraries across the ecosystem adopt this mindset? For an extreme scenario, imagine a package becomes popular that gives you a macro that logs (at trace level) every line of code in a function, including values for any expressions, which would surely ruin your time complexity.
As long as you donāt need to see any of these logs the feature as proposed is great. However, if you need to see the logs for just one library or your app, you need to have them compiled in for the whole dependency graph, which could then result in massive overhead, even if you eventually filter down the logs to just the component you are actually interested in.
Now, this is a hypothetical scenario, and I also donāt really see a way to address it with the features SwiftPM currently provides, but in a perfect world, I think the compile time level should probably default to info level for dependencies and have a way to customize it for specific packages.
I personally think this is not the last feature in the space of making no-op logs cheaper that we need to add to swift-log. Having a dependency graph wide toggle to disable a log level is IMO a good first step. Individual packages can add their own traits to conditionally compile in/out their log statements and it might be reasonable to ask swift-log for some help in making this easier. Potentially with a macro or similar. I think we should see how we far we get with these proposed "global" traits and then decide what the next step is.
Thank you for the feedback. As far as I see there is no issues left with the technical side of the proposal, it is accepted and is now Ready for Implementation.