Support `tracing` as a feature. Update parking_lot. Fix some clippy warnings.

[dtzxporter]

• Allow tracing or log based on features
• Update parking_lot to 0.12.
• Fix some clippy warnings around patterns / matches.
• Convert tests to use tracing-subscriber.

[lerouxrgd]

Hello,

Using tracing should be optional and available as a feature. I think there could be a logger macro with two implementations gated by #[cfg(feature = "tracing")] and #[cfg(not(feature = "tracing"))]. Similarly, configure_logging could be made available or not, and in case it is not regular logging should be enabled.

[dtzxporter]

Sure, what would you like the default to be, log, or tracing?

[lerouxrgd]

I think log is a better default.

[dtzxporter]

Sure, give me a few and I'll make the req changes

[dtzxporter]

Let me know if these changes work for you!

[lerouxrgd]

Let me give it a try on your branch to confirm my latest comments

[dtzxporter]

Sure, for the macro thing, I wasn't able to do so... I want macros 2.0 already :D

[lerouxrgd]

So I confirm my comments are valid, here is a patch that passes the tests:

patch.diff

```diff diff --git a/src/datachannel.rs b/src/datachannel.rs index 0929ff8..65238be 100644 --- a/src/datachannel.rs +++ b/src/datachannel.rs @@ -7,6 +7,7 @@ use std::slice; use datachannel_sys as sys; use crate::error::{check, Error, Result}; +use crate::logger; #[derive(Debug, Clone, Default)] pub struct Reliability { @@ -221,18 +222,22 @@ where Ok(_) => match crate::ffi_string(&buf) { Ok(label) => label, Err(err) => { - error!( + logger::error!( "Couldn't get label for RtcDataChannel id={} {:p}, {}", - self.id, self, err + self.id, + self, + err ); String::default() } }, Err(err) => { - warn!( + logger::warn!( "Couldn't get label for RtcDataChannel id={} {:p}, {}", - self.id, self, err + self.id, + self, + err ); String::default() } @@ -257,17 +262,21 @@ where Ok(_) => match crate::ffi_string(&buf) { Ok(protocol) => Some(protocol), Err(err) => { - error!( + logger::error!( "Couldn't get protocol for RtcDataChannel id={} {:p}, {}", - self.id, self, err + self.id, + self, + err ); None } }, Err(err) => { - warn!( + logger::warn!( "Couldn't get protocol for RtcDataChannel id={} {:p}, {}", - self.id, self, err + self.id, + self, + err ); None } @@ -302,9 +311,11 @@ where match check(unsafe { sys::rtcGetBufferedAmount(self.id) }) { Ok(amount) => amount as usize, Err(err) => { - error!( + logger::error!( "Couldn't get buffered_amount for RtcDataChannel id={} {:p}, {}", - self.id, self, err + self.id, + self, + err ); 0 } @@ -336,9 +347,11 @@ where match check(unsafe { sys::rtcGetAvailableAmount(self.id) }) { Ok(amount) => amount as usize, Err(err) => { - error!( + logger::error!( "Couldn't get available_amount for RtcDataChannel id={} {:p}, {}", - self.id, self, err + self.id, + self, + err ); 0 } @@ -349,9 +362,11 @@ where impl Drop for RtcDataChannel { fn drop(&mut self) { if let Err(err) = check(unsafe { sys::rtcDeleteDataChannel(self.id) }) { - error!( + logger::error!( "Error while dropping RtcDataChannel id={} {:p}: {}", - self.id, self, err + self.id, + self, + err ); } } diff --git a/src/lib.rs b/src/lib.rs index 55392a0..bef78f9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,11 +1,9 @@ use std::sync::Once; -#[macro_use] -mod logger; - mod config; mod datachannel; mod error; +mod logger; mod peerconnection; mod track; @@ -17,15 +15,17 @@ mod sys { use datachannel_sys as sys; - pub unsafe extern "C" fn log_callback(level: sys::rtcLogLevel, message: *const c_char) { + use crate::logger; + + pub(crate) unsafe extern "C" fn log_callback(level: sys::rtcLogLevel, message: *const c_char) { let message = CStr::from_ptr(message).to_string_lossy(); match level { sys::rtcLogLevel_RTC_LOG_NONE => (), - sys::rtcLogLevel_RTC_LOG_ERROR => error!("{}", message), - sys::rtcLogLevel_RTC_LOG_WARNING => warn!("{}", message), - sys::rtcLogLevel_RTC_LOG_INFO => info!("{}", message), - sys::rtcLogLevel_RTC_LOG_DEBUG => debug!("{}", message), - sys::rtcLogLevel_RTC_LOG_VERBOSE => trace!("{}", message), + sys::rtcLogLevel_RTC_LOG_ERROR => logger::error!("{}", message), + sys::rtcLogLevel_RTC_LOG_WARNING => logger::warn!("{}", message), + sys::rtcLogLevel_RTC_LOG_INFO => logger::info!("{}", message), + sys::rtcLogLevel_RTC_LOG_DEBUG => logger::debug!("{}", message), + sys::rtcLogLevel_RTC_LOG_VERBOSE => logger::trace!("{}", message), _ => unreachable!(), } } diff --git a/src/logger.rs b/src/logger.rs index 9587d38..0ad0e3a 100644 --- a/src/logger.rs +++ b/src/logger.rs @@ -1,50 +1,24 @@ -#[cfg(feature = "tracing")] -macro_rules! error { - ($($input:tt)*) => (::tracing::error!($($input)*)) -} - #[cfg(feature = "log")] -macro_rules! error { - ($($input:tt)*) => (::log::error!($($input)*)) -} - +pub use log::error; #[cfg(feature = "tracing")] -#[macro_export] -macro_rules! warn { - ($($input:tt)*) => (::tracing::warn!($($input)*)) -} +pub use tracing::error; #[cfg(feature = "log")] -macro_rules! warn { - ($($input:tt)*) => (::log::warn!($($input)*)) -} - +pub use log::warn; #[cfg(feature = "tracing")] -macro_rules! info { - ($($input:tt)*) => (::tracing::info!($($input)*)) -} +pub use tracing::warn; #[cfg(feature = "log")] -macro_rules! info { - ($($input:tt)*) => (::log::info!($($input)*)) -} - +pub use log::info; #[cfg(feature = "tracing")] -macro_rules! trace { - ($($input:tt)*) => (::tracing::trace!($($input)*)) -} +pub use tracing::info; #[cfg(feature = "log")] -macro_rules! trace { - ($($input:tt)*) => (::log::trace!($($input)*)) -} - +pub use log::debug; #[cfg(feature = "tracing")] -macro_rules! debug { - ($($input:tt)*) => (::tracing::debug!($($input)*)) -} +pub use tracing::debug; #[cfg(feature = "log")] -macro_rules! debug { - ($($input:tt)*) => (::log::debug!($($input)*)) -} \ No newline at end of file +pub use log::trace; +#[cfg(feature = "tracing")] +pub use tracing::trace; diff --git a/src/peerconnection.rs b/src/peerconnection.rs index b803b95..787d43d 100644 --- a/src/peerconnection.rs +++ b/src/peerconnection.rs @@ -12,6 +12,7 @@ use webrtc_sdp::{media_type::SdpMedia, parse_sdp, SdpSession}; use crate::config::RtcConfig; use crate::datachannel::{DataChannelHandler, DataChannelInit, RtcDataChannel}; use crate::error::{check, Error, Result}; +use crate::logger; use crate::track::{RtcTrack, TrackHandler, TrackInit}; #[derive(Debug, PartialEq)] @@ -248,8 +249,8 @@ where let sdp = match parse_sdp(&sdp, false) { Ok(sdp) => sdp, Err(err) => { - warn!("Ignoring invalid SDP: {}", err); - debug!("{}", sdp); + logger::warn!("Ignoring invalid SDP: {}", err); + logger::debug!("{}", sdp); return; } }; @@ -258,8 +259,8 @@ where let sdp_type = match SdpType::from(&sdp_type) { Ok(sdp_type) => sdp_type, Err(_) => { - warn!("Ignoring session with invalid SdpType: {}", sdp_type); - debug!("{}", sdp); + logger::warn!("Ignoring session with invalid SdpType: {}", sdp_type); + logger::debug!("{}", sdp); return; } }; @@ -325,9 +326,11 @@ where let _guard = rtc_pc.lock.lock(); rtc_pc.pc_handler.on_data_channel(dc); } - Err(err) => error!( + Err(err) => logger::error!( "Couldn't create RtcDataChannel with id={} from RtcPeerConnection {:p}: {}", - id, ptr, err + id, + ptr, + err ), } } @@ -413,7 +416,7 @@ where match (sdp, sdp_type) { (Some(Ok(sdp)), Some(Ok(sdp_type))) => Some(SessionDescription { sdp, sdp_type }), (Some(Err(e)), _) | (None, Some(Err(e))) => { - error!("Got an invalid Sessiondescription: {}", e); + logger::error!("Got an invalid Sessiondescription: {}", e); None } _ => None, @@ -432,7 +435,7 @@ where match (sdp, sdp_type) { (Some(Ok(sdp)), Some(Ok(sdp_type))) => Some(SessionDescription { sdp, sdp_type }), (Some(Err(e)), _) | (None, Some(Err(e))) => { - error!("Got an invalid Sessiondescription: {}", e); + logger::error!("Got an invalid Sessiondescription: {}", e); None } _ => None, @@ -461,7 +464,7 @@ where let buf_size = match buf_size { Ok(buf_size) => buf_size as usize, Err(err) => { - error!("Couldn't get buffer size: {}", err); + logger::error!("Couldn't get buffer size: {}", err); return None; } }; @@ -483,9 +486,10 @@ where match (local, remote) { (Ok(local), Ok(remote)) => Some(CandidatePair { local, remote }), (Ok(_), Err(err)) | (Err(err), Ok(_)) | (Err(err), Err(_)) => { - error!( + logger::error!( "Couldn't get RtcPeerConnection {:p} candidate_pair: {}", - self, err + self, + err ); None } @@ -493,9 +497,10 @@ where } Err(Error::NotAvailable) => None, Err(err) => { - warn!( + logger::warn!( "Couldn't get RtcPeerConnection {:p} candidate_pair: {}", - self, err + self, + err ); None } @@ -510,7 +515,7 @@ where let buf_size = match check(unsafe { str_fn(self.id, ptr::null_mut() as *mut c_char, 0) }) { Ok(buf_size) => buf_size as usize, Err(err) => { - error!("Couldn't get buffer size: {}", err); + logger::error!("Couldn't get buffer size: {}", err); return None; } }; @@ -520,18 +525,22 @@ where Ok(_) => match String::from_utf8(buf) { Ok(local) => Some(local.trim_matches(char::from(0)).to_string()), Err(err) => { - error!( + logger::error!( "Couldn't get RtcPeerConnection {:p} {}: {}", - self, prop, err + self, + prop, + err ); None } }, Err(Error::NotAvailable) => None, Err(err) => { - warn!( + logger::warn!( "Couldn't get RtcPeerConnection {:p} {}: {}", - self, prop, err + self, + prop, + err ); None } @@ -542,9 +551,11 @@ where impl

Drop for RtcPeerConnection

{ fn drop(&mut self) { if let Err(err) = check(unsafe { sys::rtcDeletePeerConnection(self.id) }) { - error!( + logger::error!( "Error while dropping RtcPeerConnection id={} {:p}: {}", - self.id, self, err + self.id, + self, + err ) } } diff --git a/src/track.rs b/src/track.rs index 34fcfe0..8916780 100644 --- a/src/track.rs +++ b/src/track.rs @@ -5,6 +5,7 @@ use std::slice; use datachannel_sys as sys; use crate::error::{check, Result}; +use crate::logger; #[derive(Debug, Clone, Copy)] pub enum Direction { @@ -168,9 +169,11 @@ where impl Drop for RtcTrack { fn drop(&mut self) { if let Err(err) = check(unsafe { sys::rtcDeleteTrack(self.id) }) { - error!( + logger::error!( "Error while dropping RtcTrack id={} {:p}: {}", - self.id, self, err + self.id, + self, + err ); } } ```

[dtzxporter]

Ahh yes, you can do that when you re-export the existing macros, I was confirming to your original comment about "making a macro" which doesn't work the way you described.

[lerouxrgd]

However there is an issue with the current approach:

# works
cargo test

# works
cargo test --no-default-features --features tracing

# does NOT work
cargo test  --features tracing

# does NOT work
cargo test --no-default-features

So all the above have to work. I'll try to think of something for that.

[lerouxrgd]

Ahh yes, you can do that when you re-export the existing macros, I was confirming to your original comment about "making a macro" which doesn't work the way you described.

You're right I wasn't clear, sorry for that.

[dtzxporter]

Perhaps we take inspiration from libraries that have async-std/tokio support? A custom error message:

#[cfg(not(any(
    feature = "runtime-actix-native-tls",
    feature = "runtime-async-std-native-tls",
    feature = "runtime-tokio-native-tls",
    feature = "runtime-actix-rustls",
    feature = "runtime-async-std-rustls",
    feature = "runtime-tokio-rustls",
)))]
compile_error!(
    "one of the features ['runtime-actix-native-tls', 'runtime-async-std-native-tls', \
     'runtime-tokio-native-tls', 'runtime-actix-rustls', 'runtime-async-std-rustls', \
     'runtime-tokio-rustls'] must be enabled"
);

#[cfg(any(
    all(feature = "_rt-actix", feature = "_rt-async-std"),
    all(feature = "_rt-actix", feature = "_rt-tokio"),
    all(feature = "_rt-async-std", feature = "_rt-tokio"),
    all(feature = "_tls-native-tls", feature = "_tls-rustls"),
))]
compile_error!(
    "only one of ['runtime-actix-native-tls', 'runtime-async-std-native-tls', \
     'runtime-tokio-native-tls', 'runtime-actix-rustls', 'runtime-async-std-rustls', \
     'runtime-tokio-rustls'] can be enabled"
);

[lerouxrgd]

That's a good idea !

[dtzxporter]

Added + reworked to just reexport the macros from logger.

[lerouxrgd]

I think there are some

#[cfg(feature = "log")]
use log::*;
#[cfg(feature = "tracing")]
use tracing::*;

remaining in the tests

[dtzxporter]

I think there are some

#[cfg(feature = "log")]
use log::*;
#[cfg(feature = "tracing")]
use tracing::*;

remaining in the tests

So I can import logging but only if I make it public from datachannel... crate in a test is it's own module.

[lerouxrgd]

Maybe:

#[cfg(feature = "log")]
use log as logger;
#[cfg(feature = "tracing")]
use tracing as logger;

?

[lerouxrgd]

Thanks ! I'll release it later today.

[lerouxrgd]

Released in 0.7.4