Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternative field wrappers in tracing-attributes #1089

Open
kjvalencik opened this issue Nov 4, 2020 · 4 comments
Open

Alternative field wrappers in tracing-attributes #1089

kjvalencik opened this issue Nov 4, 2020 · 4 comments
Labels
crate/attributes Related to the `tracing-attributes` crate kind/feature New feature or request

Comments

@kjvalencik
Copy link

kjvalencik commented Nov 4, 2020

Feature Request

Crates

tracing-attributes

Motivation

I am working on a security sensitive application where I do not want to leak information in logs or traces. Since many types implement Debug, it is very difficult to manage the security of this trait. However, it is intricately tied to the instrument proc-macro.

Ideally, I would be able to wrap the macro with customization to use a field wrapper other than tracing::field::debug without forking tracing-attributes.

Proposal

The primary implementation of instrument be moved into a struct with the name of the wrapper fn as a key.

pub struct Instrument {
    field_wrapper: String,
}

impl Default for Instrument {
    fn default() -> Self {
        Self { field_wrapper: String::from("tracing::field::debug") }
    }
}

impl Instrument {
    pub fn set_field_wrapper(&mut self, field_wrapper: String) {
         self.field_wrapper = String;
    }

    pub fn instrument(
        &self,
        args: proc_macro::TokenStream,
        item: proc_macro::TokenStream,
    ) -> proc_macro::TokenStream {
        todo!()
    }
}

#[proc_macro_attribute]
pub fn instrument(
    args: proc_macro::TokenStream,
    item: proc_macro::TokenStream,
) -> proc_macro::TokenStream {
    Instrument::default().instrument(args, item)
}

Then, from my own code, I can implement a wrapper that relies on my log safe trait.

trait Loggable {
    fn format(&self, f: &mut fmt::Formatter) -> fmt::Result;
}

pub fn loggable<T>(t: T) -> tracing::field::DebugValue<LoggableValue<T>>
where
    T: Loggable,
{
    tracing::field::debug(LoggableValue(t))
}

pub struct LoggableValue<T: Loggable>(T);

impl<T: Loggable> std::fmt::Debug for LoggableValue<T> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        Loggable::format(&self.0, f)
    }
}

#[proc_macro_attribute]
pub fn instrument(
    args: proc_macro::TokenStream,
    item: proc_macro::TokenStream,
) -> proc_macro::TokenStream {
    let mut instrument = Instrument::default();
    instrument.set_field_wrapper(String::from("my_crate::field::loggable"));
    instrument.instrument(args, item)
}

Alternatives

  • Forking tracing-attributes
  • Not using the instrument macro
  • Configuration on every use of the macro
@hawkw
Copy link
Member

hawkw commented Nov 4, 2020

Thanks for the suggestion, this is an interesting idea!

Note, though, that you can also do this by skipping all the automatically-generated fields, and adding custom fields using your own wrapper type. For example:

#[tracing::instrument(
    skip(foo, bar),
    fields(foo = ?my_wrapper(foo), bar = ?my_wrapper(bar))
)]
fn my_function(foo: Foo, bar: Bar) {
   /// ...
}

or similar. You could also have your own custom instrument attribute that wraps #[tracing::instrument] to do this automatically, without having to add an API for customizing a field wrapper.

Also, in the upcoming tracing 0.2 release, the #[instrument] attribute will no longer automatically record all arguments by default. Instead, all fields will be opt in rather than opt-out, and can be arbitrary expressions, using the same syntax as the function-like macros. So, the skip argument will no longer be necessary, as fields will not be added by default.

@kjvalencik
Copy link
Author

@hawkw Wrapping may be an option, but I think it would still require a slight refactoring to expose the implementation in a pub fn that isn't a macro. Removing default arguments is fantastic, but unfortunately that is insufficient of a guarantee for my use case because it would require auditing all #[instrument(...)] attributes for proper wrapping.

Thanks for considering my use case!

@hawkw hawkw added the kind/feature New feature or request label Nov 6, 2020
@hawkw
Copy link
Member

hawkw commented Nov 6, 2020

Wrapping may be an option, but I think it would still require a slight refactoring to expose the implementation in a pub fn that isn't a macro.

Hmm, can you explain why? I think wrapper proc macro could just expand to include a #[tracing::instrument] attribute that does what you want it to do.

I'm definitely not opposed to the option of exposing the instrument macro in a public function — that could potentially make it easier for folks to build custom instrumentation macros and experiment with different behaviors, which would be very cool! However, it would take some thinking about what functions it would make sense to include in a stable API for the proc macro internals. I'd like to be able to support more use-cases for custom/experimental proc macros than just being able to wrap fields in an additional function.

Since this could take a little time to design the ideal API for the proc macro internals to expose, I'd like to make sure you have a workaround or stopgap solution that works for your use case in the interim!

@hawkw hawkw added the crate/attributes Related to the `tracing-attributes` crate label Nov 6, 2020
@kjvalencik
Copy link
Author

kjvalencik commented Nov 6, 2020

@hawkw Yes, I have a workaround. Since the crate specifies tracing as the module instead of ::tracing, I'm able to inject use my_crate::tracing; into the function bodies to work around the issue.

With that said, that feels like something that probably should be changed since it could equally cause unexpected issues for users, but I'm good for now! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/attributes Related to the `tracing-attributes` crate kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants