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

Add ignored parameters with #[trace_ignore] #11

Closed
kleimkuhler opened this issue Feb 10, 2019 · 9 comments · Fixed by #359
Closed

Add ignored parameters with #[trace_ignore] #11

kleimkuhler opened this issue Feb 10, 2019 · 9 comments · Fixed by #359
Labels
crate/attributes Related to the `tracing-attributes` crate good first issue Good for newcomers kind/feature New feature or request

Comments

@kleimkuhler
Copy link
Contributor

As mentioned in #5:

...
Since some parameters may not be valid span fields, we might consider adding an additional attribute (#[trace_ignore]?) to annotate those fields that should be skipped.

I think this could be easier to add now that parameters are passed into span!. @hawkw was what you had in mind for this something like this?

#[trace]
#[trace_ignore(bar, baz)]
fn example(foo: t1, bar: t2, bar: t3) -> t4 {
    ...
}
@hawkw
Copy link
Member

hawkw commented Feb 10, 2019

@kleimkuhler that seems reasonable. Alternatively, we could place the attribute on the argument to ignore, as in:

#[trace]
fn example(foo: t1, #[trace_ignore] bar: t2, #[trace_ignore] baz: t3) -> t4 {
    ...
}

though I'm not sure if there's existing precedent for attributes on args?

@hawkw
Copy link
Member

hawkw commented Feb 10, 2019

Yet another option would be passing the ignored fields as an argument to the #[trace] attribute, like

#[trace(ignore = bar, baz)]

though that seems potentially confusing.

We could also just make the #[trace] attribute opt-in, so you could say

#[trace(foo)]
fn example(foo: t1, bar: t2, baz: t3) -> t4 {
    ...
}

to ignore bar and baz. Using #[trace] without any arguments would be equivalent to passing all the function's parameters.

I'm not sure which is the most ergonomic syntax --- @davidbarsky, any opinions?

@hawkw hawkw added kind/feature New feature or request crate/attributes Related to the `tracing-attributes` crate labels Feb 10, 2019
@kleimkuhler
Copy link
Contributor Author

Of those options I think #[trace(...)] is most reasonable. I don't really like the way tagging individual parameters looks.

Opt-in vs opt-out is something I'd be interested to hear @davidbarsky thoughts on! I'm not really sure if one makes sense over the other.

@hawkw
Copy link
Member

hawkw commented Feb 10, 2019 via email

@hawkw hawkw added the good first issue Good for newcomers label Jul 3, 2019
@hawkw hawkw added this to the tracing-macros 0.1 milestone Jul 8, 2019
@anp
Copy link
Contributor

anp commented Aug 17, 2019

(assuming trace -> instrument, please correct me if I'm wrong)

Using the macro a little bit I've found that I want nearly all of the arguments in functions where it's used. Based on this, I'd have a preference for an opt-out argument to the macro. I'd also have a preference for the shortest argument name possible, something like skip or not.

@carllerche
Copy link
Member

Makes sense to me!

@hdevalence
Copy link
Contributor

I'd be happy to work on a PR for this, using skip as suggested above. I'm not sure what the best way to specify multiple arguments-to-skip is.

Would multiple skip parameters work, as in

#[instrument(skip = b, skip = c)]
fn instrumented_fn(a: A, b: B, c: C) { /* ... */ }

@hawkw
Copy link
Member

hawkw commented Sep 23, 2019

I'd be happy to work on a PR for this, using skip as suggested above. I'm not sure what the best way to specify multiple arguments-to-skip is.

Would multiple skip parameters work, as in

#[instrument(skip = b, skip = c)]
fn instrumented_fn(a: A, b: B, c: C) { /* ... */ }

@hdevalence I'm fine with that proposal. As an alternative, we could also do something like:

#[instrument(skip(a, b))]
fn instrumented_fn(a: A, b: B, c: C) { /* ... */ }

possibly using something like syn's MetaList. I'm open to either syntax — what do you think feels more natural?

@hdevalence
Copy link
Contributor

@hawkw the second syntax seems better as a user, thanks for the pointer to MetaList.

hawkw pushed a commit that referenced this issue Sep 23, 2019
## Motivation

This adds a `skip` directive to the `#[instrument]` macro that allows
specifying one or more arguments to the function which will not appear
in the generated span.  In particular, this means that it's possible to
use `#[instrument]` on functions with a non-`Debug` parameter.

## Solution

The interior of a `skip(arg1, arg2)` argument is scanned for identifiers
and these are filtered out of the identifier list used to generate the
span.

Closes #11.
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 good first issue Good for newcomers kind/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants