Skip to content

Commit

Permalink
Build returns a Result
Browse files Browse the repository at this point in the history
- LayerBuilder::build() now returns a Result rather than panics
  - Add new error type
  - Update samples
- Remove global_filter feature
  - Global filter is now a runtime option: LayerBuilder::build_global_filter
  • Loading branch information
Robo210 committed Aug 22, 2024
1 parent 3663164 commit f218689
Show file tree
Hide file tree
Showing 19 changed files with 82 additions and 63 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ authors = ["Kyle Sabo", "Microsoft"]
crate-type = ["rlib"]

[features]
global_filter = []
common_schema = []
default = ["common_schema"]

Expand All @@ -24,6 +23,7 @@ chrono = {version="0.4", default-features = false, features=["std"]}
once_cell = ">=1.18"
dashmap = "6"
paste = "1"
thiserror = "1"

[target.'cfg(not(target_os = "linux"))'.dependencies]
tracelogging = ">= 1.2.0"
Expand Down
2 changes: 1 addition & 1 deletion benches/etw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use tracing_subscriber::{self, prelude::*};
pub fn etw_benchmark(c: &mut Criterion) {
let builder = LayerBuilder::new("etw_bench");
let provider_id = builder.get_provider_id();
let _subscriber = tracing_subscriber::registry().with(builder.build()).init();
let _subscriber = tracing_subscriber::registry().with(builder.build().unwrap()).init();

let etw_session = SessionBuilder::new_file_mode(
"tokio-tracing-etw-bench",
Expand Down
2 changes: 1 addition & 1 deletion benches/user_events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use tracing_subscriber::{self, prelude::*};
#[cfg(target_os = "linux")]
pub fn user_events_benchmark(c: &mut Criterion) {
let builder = LayerBuilder::new("user_events_bench");
let _subscriber = tracing_subscriber::registry().with(builder.build()).init();
let _subscriber = tracing_subscriber::registry().with(builder.build().unwrap()).init();

// Disabled provider
// {
Expand Down
4 changes: 2 additions & 2 deletions examples/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use tracing_subscriber::{self, fmt::format::FmtSpan, prelude::*};

fn main() {
tracing_subscriber::registry()
.with(LayerBuilder::new("ExampleProvBasic").build()) // Collects everything
.with(LayerBuilder::new_common_schema_events("ExampleProvBasic_CS").build_with_target("geneva"))
.with(LayerBuilder::new("ExampleProvBasic").build().unwrap()) // Collects everything
.with(LayerBuilder::new_common_schema_events("ExampleProvBasic_CS").build_with_target("geneva").unwrap())
.with(tracing_subscriber::fmt::layer().with_span_events(FmtSpan::ACTIVE))
.init();

Expand Down
2 changes: 1 addition & 1 deletion examples/etw_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use tracing_subscriber::{self, fmt::format::FmtSpan, prelude::*};

fn main() {
tracing_subscriber::registry()
.with(LayerBuilder::new_common_schema_events("ExampleProvEtwEvent_CS").build())
.with(LayerBuilder::new_common_schema_events("ExampleProvEtwEvent_CS").build().unwrap())
.with(tracing_subscriber::fmt::layer().with_span_events(FmtSpan::ACTIVE))
.init();

Expand Down
2 changes: 1 addition & 1 deletion examples/instrument.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ fn test_function(x: u32, y: f64) -> f64 {

fn main() {
tracing_subscriber::registry()
.with(LayerBuilder::new("ExampleProvInstrument").build()) // Collects everything
.with(LayerBuilder::new("ExampleProvInstrument").build().unwrap()) // Collects everything
.with(tracing_subscriber::fmt::layer().with_span_events(FmtSpan::ACTIVE))
.init();

Expand Down
2 changes: 1 addition & 1 deletion examples/span.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use tracing_subscriber::{self, fmt::format::FmtSpan, prelude::*};

fn main() {
tracing_subscriber::registry()
.with(LayerBuilder::new_common_schema_events("ExampleProvSpan_CS").build())
.with(LayerBuilder::new_common_schema_events("ExampleProvSpan_CS").build().unwrap())
.with(tracing_subscriber::fmt::layer().with_span_events(FmtSpan::ACTIVE))
.init();

Expand Down
2 changes: 1 addition & 1 deletion examples/stress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use tracing_subscriber::{self, prelude::*};

fn main() {
tracing_subscriber::registry()
.with(LayerBuilder::new("ExampleProvEtwEventStress").build())
.with(LayerBuilder::new("ExampleProvEtwEventStress").build().unwrap())
.init();

for i in 0..500000 {
Expand Down
1 change: 0 additions & 1 deletion src/_details.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ where

// This struct needs to be public as it implements the tracing_subscriber::Layer::Filter trait.
#[doc(hidden)]
#[cfg(any(not(feature = "global_filter"), docsrs))]
pub struct EtwFilter<S, Mode: ProviderTypes>
where
Mode::Provider: crate::native::EventWriter<Mode> + 'static
Expand Down
4 changes: 2 additions & 2 deletions src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use tracing_subscriber::{self, fmt::format::FmtSpan, prelude::*};

fn main() {
tracing_subscriber::registry()
.with(LayerBuilder::new("test").build()) // Collects everything
.with(LayerBuilder::new_common_schema_events("test2").build_with_target("geneva"))
.with(LayerBuilder::new("test").build().unwrap()) // Collects everything
.with(LayerBuilder::new_common_schema_events("test2").build_with_target("geneva").unwrap())
.with(tracing_subscriber::fmt::layer().with_span_events(FmtSpan::ACTIVE))
.init();

Expand Down
11 changes: 11 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
use thiserror::Error;

#[derive(Error, Debug)]
pub enum EtwError {
#[error("Provider group GUID must not be zeroes")]
EmptyProviderGroupGuid,
#[error("Provider group names must be lower case ASCII or numeric digits: {0:?}")]
InvalidProviderGroupCharacters(String),
#[error("Linux provider names must be ASCII alphanumeric: {0:?}")]
InvalidProviderNameCharacters(String)
}
50 changes: 18 additions & 32 deletions src/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ use tracing_subscriber::filter::{combinator::And, FilterExt, Filtered, Targets};
use tracing_subscriber::layer::Filter;
use tracing_subscriber::{registry::LookupSpan, Layer};

#[cfg(any(not(feature = "global_filter"), docsrs))]
use crate::_details::EtwFilter;
use crate::_details::EtwLayer;
use crate::_details::{EtwFilter, EtwLayer};
use crate::native::{EventWriter, GuidWrapper, ProviderTypes};
use crate::native;
use crate::{error::EtwError, native};
use crate::values::*;
use crate::statics::*;

Expand Down Expand Up @@ -128,24 +126,23 @@ where
self
}

fn validate_config(&self) {
match &self.provider_group {
None => (),
Some(value) => Mode::assert_valid(value)
}

fn validate_config(&self) -> Result<(), EtwError> {
#[cfg(target_os = "linux")]
if self
.provider_name
.contains(|f: char| !f.is_ascii_alphanumeric())
{
// The perf command is very particular about the provider names it accepts.
// The Linux kernel itself cares less, and other event consumers should also presumably not need this check.
//panic!("Linux provider names must be ASCII alphanumeric");
return Err(EtwError::InvalidProviderNameCharacters(self.provider_name.clone()));
}

match &self.provider_group {
None => Ok(()),
Some(value) => Mode::is_valid(value)
}
}

#[cfg(any(not(feature = "global_filter"), docsrs))]
fn build_target_filter(&self, target: &'static str) -> Targets {
let mut targets = Targets::new().with_target(&self.provider_name, LevelFilter::TRACE);

Expand Down Expand Up @@ -181,7 +178,6 @@ where
}
}

#[cfg(not(feature = "global_filter"))]
fn build_filter<S>(&self, provider: Pin<Arc<Mode::Provider>>) -> EtwFilter<S, Mode>
where
S: Subscriber + for<'a> LookupSpan<'a>,
Expand All @@ -195,62 +191,55 @@ where
}
}

#[cfg_attr(docsrs, doc(cfg(feature = "global_filter")))]
#[cfg(any(feature = "global_filter", docsrs))]
pub fn build<S>(self) -> EtwLayer<S, Mode>
pub fn build_global_filter<S>(self) -> Result<EtwLayer<S, Mode>, EtwError>
where
S: Subscriber + for<'a> LookupSpan<'a>,
Mode::Provider: EventWriter<Mode> + 'static,
{
self.validate_config();
self.validate_config()?;

self.build_layer()
Ok(self.build_layer())
}

#[allow(clippy::type_complexity)]
#[cfg_attr(docsrs, doc(cfg(not(feature = "global_filter"))))]
#[cfg(any(not(feature = "global_filter"), docsrs))]
pub fn build<S>(self) -> Filtered<EtwLayer<S, Mode>, EtwFilter<S, Mode>, S>
pub fn build<S>(self) -> Result<Filtered<EtwLayer<S, Mode>, EtwFilter<S, Mode>, S>, EtwError>
where
S: Subscriber + for<'a> LookupSpan<'a>,
Mode::Provider: EventWriter<Mode> + 'static,
{
self.validate_config();
self.validate_config()?;

let layer = self.build_layer();

let filter = self.build_filter(layer.provider.clone());

layer.with_filter(filter)
Ok(layer.with_filter(filter))
}

/// Constructs the configured layer with a target [tracing_subscriber::filter] applied.
/// This can be used to target specific events to specific layers, and in effect allow
/// specific events to be logged only from specific ETW/user_event providers.
#[allow(clippy::type_complexity)]
#[cfg_attr(docsrs, doc(cfg(not(feature = "global_filter"))))]
#[cfg(any(not(feature = "global_filter"), docsrs))]
pub fn build_with_target<S>(
self,
target: &'static str,
) -> Filtered<EtwLayer<S, Mode>, And<EtwFilter<S, Mode>, Targets, S>, S>
) -> Result<Filtered<EtwLayer<S, Mode>, And<EtwFilter<S, Mode>, Targets, S>, S>, EtwError>
where
S: Subscriber + for<'a> LookupSpan<'a>,
Mode::Provider: EventWriter<Mode> + 'static,
{
self.validate_config();
self.validate_config()?;

let layer = self.build_layer();

let filter = self.build_filter(layer.provider.clone());

let targets = self.build_target_filter(target);

layer.with_filter(filter.and(targets))
Ok(layer.with_filter(filter.and(targets)))
}
}

#[cfg(any(not(feature = "global_filter"), docsrs))]
impl<S, Mode> Filter<S> for EtwFilter<S, Mode>
where
S: Subscriber + for<'a> LookupSpan<'a>,
Expand Down Expand Up @@ -335,7 +324,6 @@ where
// Late init when the layer is attached to a subscriber
}

#[cfg(any(feature = "global_filter", docsrs))]
fn register_callsite(
&self,
metadata: &'static tracing::Metadata<'static>,
Expand All @@ -360,7 +348,6 @@ where
}
}

#[cfg(any(feature = "global_filter", docsrs))]
fn enabled(
&self,
metadata: &tracing::Metadata<'_>,
Expand All @@ -376,7 +363,6 @@ where
self.provider.enabled(metadata.level(), keyword)
}

#[cfg(any(feature = "global_filter", docsrs))]
fn event_enabled(
&self,
event: &tracing::Event<'_>,
Expand Down
3 changes: 2 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
//! use tracing_subscriber::{self, prelude::*};
//!
//! tracing_subscriber::registry()
//! .with(tracing_etw::LayerBuilder::new("SampleProviderName").build())
//! .with(tracing_etw::LayerBuilder::new("SampleProviderName").build().unwrap())
//! .init();
//!
//! event!(Level::INFO, fieldB = b'x', fieldA = 7, "Event Message!");
Expand Down Expand Up @@ -85,6 +85,7 @@ mod statics;
// Module holding internal details that need to be public but should not be directly used by consumers of the crate.
#[doc(hidden)]
pub mod _details;
pub mod error;

pub use layer::LayerBuilder;

Expand Down
11 changes: 8 additions & 3 deletions src/native/common_schema/etw_cs.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::values::*;
use crate::{error::EtwError, values::*};
use std::{
cell::RefCell,
io::{Cursor, Write},
Expand Down Expand Up @@ -88,8 +88,13 @@ impl crate::native::ProviderTypes for CommonSchemaProvider {
true
}

fn assert_valid(value: &Self::ProviderGroupType) {
assert_ne!(value, &tracelogging_dynamic::Guid::zero(), "Provider group GUID must not be zeroes");
fn is_valid(value: &Self::ProviderGroupType) -> Result<(), EtwError> {
if value == &crate::native::native_guid::zero() {
Err(EtwError::EmptyProviderGroupGuid)
}
else {
Ok(())
}
}
}

Expand Down
14 changes: 9 additions & 5 deletions src/native/common_schema/user_events_cs.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::values::*;
use crate::error::EtwError;
use eventheader::*;
use eventheader_dynamic::EventBuilder;
use std::{
Expand Down Expand Up @@ -57,11 +58,14 @@ impl crate::native::ProviderTypes for CommonSchemaProvider {
false
}

fn assert_valid(value: &Self::ProviderGroupType) {
assert!(
eventheader_dynamic::ProviderOptions::is_valid_option_value(value),
"Provider group names must be lower case ASCII or numeric digits"
);
fn is_valid(value: &Self::ProviderGroupType) -> Result<(), EtwError> {
if !eventheader_dynamic::ProviderOptions::is_valid_option_value(value) {
Err(EtwError::InvalidProviderGroupCharacters(value.clone().into()))
}
else
{
Ok(())
}
}

fn get_provider_group(value: &Self::ProviderGroupType) -> impl Into<String> {
Expand Down
11 changes: 8 additions & 3 deletions src/native/etw.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::values::*;
use crate::{error::EtwError, values::*};
use crate::statics::GLOBAL_ACTIVITY_SEED;
use chrono::{Datelike, Timelike};
use std::{cell::RefCell, ops::DerefMut, pin::Pin, sync::Arc, time::SystemTime};
Expand Down Expand Up @@ -98,8 +98,13 @@ impl crate::native::ProviderTypes for Provider {
true
}

fn assert_valid(value: &Self::ProviderGroupType) {
assert_ne!(value, &crate::native::native_guid::zero(), "Provider group GUID must not be zeroes");
fn is_valid(value: &Self::ProviderGroupType) -> Result<(), EtwError> {
if value == &crate::native::native_guid::zero() {
Err(EtwError::EmptyProviderGroupGuid)
}
else {
Ok(())
}
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/native/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ pub(crate) use tracelogging_dynamic::Guid as native_guid;
#[cfg(target_os = "linux")]
pub(crate) use eventheader::Guid as native_guid;

use crate::error::EtwError;

#[doc(hidden)]
#[derive(Copy, Clone)]
pub struct GuidWrapper(u128);
Expand Down Expand Up @@ -92,7 +94,7 @@ pub trait ProviderTypes {

fn supports_enable_callback() -> bool;

fn assert_valid(value: &Self::ProviderGroupType);
fn is_valid(value: &Self::ProviderGroupType) -> Result<(), EtwError>;

// The compiler can't see through a 'type' within a trait to tell
// that it trivially matches a constraint unless we lower the constraint
Expand Down
4 changes: 3 additions & 1 deletion src/native/noop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ impl crate::native::ProviderTypes for Provider {
false
}

fn assert_valid(_: &Self::ProviderGroupType) {}
fn is_valid(value: &Self::ProviderGroupType) -> Result<(), EtwError> {
Ok(())
}
}

impl crate::native::EventWriter<Provider> for Provider {
Expand Down
Loading

0 comments on commit f218689

Please sign in to comment.