Use more log::warns in the code

Manishearth commented 2 years ago

We have a bunch of debug asserts that could probably be log::warn for better user experience.

Perhaps also have a strict_assert feature that switches between warning and panicking for testing purposes,.

robertbastian commented 1 year ago

+1 to logging, -1 to panicking based on a feature. We should condition on debug_assertions and say we're panic-free in release mode.

sffc commented 1 year ago

Should the warnings be behind a feature? Should we route all the warnings through an intermediate crate like icu_provider?

Discuss with:

@sffc
@Manishearth
@zbraniecki

Manishearth commented 1 year ago

+1 behind a feature, am ambivalent about routing through icu_provider but I think that makes sense as a global toggle instead of adding features everywhere

sffc commented 1 year ago

@robertbastian - People don't normally depend on the icu_provider crate. We have cases already where people need to add that dep just to enable the feature.
@zbraniecki - I think I agree, but separately, is there a reason you choose logging over tracing?
@Manishearth - I think it makes sense to have a feature on the metacrate, but I think it should go through icu_provider. Even though the log crate is low overhead when you disable it, it's not completely free. I'm okay people depending on icu_provider for patterns like this. Maybe we should make icu_core which is the public API of cross things. But I think icu_provider is fine.
@robertbastian - I like re-exporting the feature on the metacrate; we could also do it for sync.
@Manishearth - On the topic of tracing... it's an independent crate in the tokio space that does tracing but better. Tracing is much more structured and path-based and you get a lot more context. The tracing crate implements that. The reason we shouldn't use it, I think, is that as a library it makes sense to use something more low-level. We could perhaps expose it as a toggle.
@zbraniecki - rust compiler switched to tracing 3 y ago - https://github.com/rust-lang/compiler-team/issues/331
@Manishearth - Yeah but the Rust compiler is an application.
@zbraniecki - Tracing provides better behavior in async contexts.
@sffc - Does tracing let us do the same feature gating as log?
@Manishearth - Yes
@sffc - Datagen is one of the main places we use logging so async support seems valuable
@zbraniecki - Agree
@sffc - Since we already have global features like serde and std, we could consider sync and tracing as more examples.
@Manishearth - I don't like polluting our feature space. We could just export a macro from icu_provider that gives you a feature-gated log.
@zbraniecki - I think it's good to not let people selectively log.
@Manishearth - Once you have logging enabled globally, then you can start filtering.
@robertbastian - I agree with a global toggle.

@Manishearth proposal: the feature exists in 2 places: icu and icu_provider (or could be icu_core). When you enable it, everyone in your dependency tree gets it.

@robertbastian - The serde feature enables additional dependencies, so it's not the same thing.
@Manishearth - Yeah, here we're exposing additional functionality, not additional dependencies.
@sffc - If we mediate the feature through icu_provider, then you don't have access to all the functionality from crates. datagen, for example, probably wants the full functionaity.
@Manishearth - datagen could have the feature.
@sffc - Can we also get rid of icu_provider/log_error_context and just group it with the main feature?
@sffc - What should the feature be called?

Conclusion:

Add features icu/logging, icu_provider/logging, and icu_datagen/logging
Enable all other logging-related features from the logging feature
Library crates should use the common logging utilities exported by icu_provider, but binary packages can use the logging crates directly
For now, continue with the log crate, but we can upgrade it later, still using the same logging feature name

LGTM: @sffc @Manishearth @robertbastian @skius @zbraniecki

robertbastian commented 1 year ago

icu_datagen currently doesn't have a logging feature and logging is a pretty fundamental part of its behaviour. Logging can actually be turned off by not registering a logger, so I'm not sure we need a separate feature...

sffc commented 1 year ago

It would be nice if all ICU4X docs tests and unit tests would have logging enabled. Currently we have logs in the code to catch possibly-errorful conditions, but those logs are suppressed, making it difficult to debug (@atcupps).

Context: The use case is that if we don't recognize a calendar BCP-47 identifier, we fall back to the locale's default calendar, which is correct, expected logic, and we have a warning printed when this fallback occurs, but that warning is not being printed in unit tests.

unicode-org / icu4x

Use more log::warns in the code #2648