instrument macro: omit self by default

[carllerche]

Feature Request

The #[instrument] proc macro should probably omit self by default as it is pretty noisy.

If #650 happens, this could be used as a way to get access to self again.

[hawkw]

I'm definitely open to this --- skipping big self arguments was one of the main motivations behind adding skip in #11.

I'd like to get input from others currently using instrument, though; ideally, the default should be whatever a majority of users want. cc @davidbarsky, @anp, @Ralith, @LucioFranco, @yaahc; I'm sure at least some of you have opinions.

Also, I think we'd need to work out whether or not this should be considered a breaking change. Existing code will still compile, but it won't behave as it did previously. Is this okay to do in a point release?

[davidbarsky]

I'd like to get input from others currently using instrument, though; ideally, the default should be whatever a majority of users want. cc @davidbarsky, @anp, @Ralith, @LucioFranco, @yaahc; I'm sure at least some of you have opinions.

I'm also in favor of this change. I think that, especially in Chalk, including &self is far too noisy. On a different note, I think this is where per-field levels might come in handy.

Also, I think we'd need to work out whether or not this should be considered a breaking change. Existing code will still compile, but it won't behave as it did previously. Is this okay to do in a point release?

I think this should be treated as a breaking change. I think (@hawkw and I) discussed that format changes should be considered breaking changes, regardless if they're additive.

[LucioFranco]

I am +1 on this as well.

[yaahc]

big +1, i skip self constantly.

What would the syntax for unskipping it be though? Ideas I can imagine

• #[instrument(skip())] - empty skip overrides the default skip
• #[instrument(self)] - self is a keyword that gets added to affirmatively include it

[hawkw]

What would the syntax for unskipping it be though?

@yaahc I think the ideal solution would just be adding self as something that can be added to the fields() argument, as in

#[instrument(fields(self))]

This could be implemented by special-casing self in particular, or by allowing arbitrary expressions to be evaluated in fields(...), as described in #650.

[yaahc]

So there would be fields and skip, and every arg other than self is automatically a field, and can be disabled with skip, and anything in fields is added to the existing fields from the non skipped args? (sgtm)

[anp]

+1 from me, I tend to think of methods happening "within the span" of self. A way to opt back in sgtm as well.

[hawkw]

Seems like most folks agree that this is the right thing, so we should probably move forwards with it.

Re: whether or not to consider it a breaking change

I think this should be treated as a breaking change. I think (@hawkw and I) discussed that format changes should be considered breaking changes, regardless if they're additive.

@davidbarsky IIRC, this conversation was largely in re: fmt::Subscriber's JSON output and other formats intended to be machine-readable. I'm not sure if I think changes in what data is recorded ought to be considered breaking; only changes in the shape of the output data (that might break parsers etc). Otherwise, if we expected libraries using tracing to uphold the same stability policy, any time you add or remove a field from an event, you would be making a breaking change, which seems nonsensical...

[anp]

We think of structured diagnostics data as an ad-hoc ABI whenever you take dependencies on it. I’m not sure that means it’s a semver breaking change to do this but it’s definitely something which (in a future with more usage) could cause broken tests when rolling versions.

[yaahc]

I personally think that its okay to expect the library that implements the logging format to have a different stability policy for log output than the applications/libraries that consume and expose it.

But I also want this asap so if making it a breaking change means its gonna take a while because we will need to batch the change with other upcoming breakages then I'd vote we just treat it as irrelevant to semver.

[hawkw]

Another thing we could do is release an 0.2x version of tracing-attributes while continuing to export 0.1 from tracing. This would mean that users who want the new "skip self by default" behavior could opt-in to it by depending on tracing-attributes 0.2 explicitly, while the re-exported version in tracing-subscriber wouldn't change its default behavior until we have other breaking changes to make.

I'm not sure if this is worth the effort.

[yaahc]

Assigning this to myself and starting to work on this now. Based on discussions with @hawkw I think we're going to try just skipping all fields by default and make logging of function parameters entirely opt in as of tracing 0.2.

Plan description

• remove skips in favor of fields
  • update all references to skips in the source and documentation, making sure to check other files beyond the lib.rs in tracing-attributes
• update logic in https://github.com/tokio-rs/tracing/blob/master/tracing-attributes/src/lib.rs#L382-L413 to start with an empty set of fields and only add them if they're mentioned in args.fields

Questions

• should there be a way to capture all args without naming them all?

[LukeMathWalker]

This issue is particularly relevant for us at TrueLayer (for reasons along the same line of those expressed in #1089).
I'd be happy to work on this if you didn't have the time to pick it up in the end @yaahc.

[yaahc]

I'd be happy to work on this if you didn't have the time to pick it up in the end @yaahc.

That would be lovely actually, let me know if you have any questions about where I left it.

[LukeMathWalker]

I have a first draft in #1306 - there are a couple of open questions to resolve, then I can start the big piece work of updating documentation/examples/etc.

[Kinrany]

Agree that skipping everything by default is better. The issue should be renamed perhaps?