attributes: add support for custom levels for ret and err (#2330)

[nitnelave]

Fixes #2330

The change is fairly straightforward, just adding support for customizable levels for ret and err in a fully backward-compatible way.

I made Level pub(crate) to be able to expose level_to_tokens, there may be a better way of doing it but it seemed simple enough and anyway it doesn't escape the crate.

[hawkw]

Thanks for working on this! This branch looks like a good start. I commented on some code style suggestions, but some of them might require a larger refactoring of this code, so if you'd prefer not do make all of those changes as part of this PR, that's fine. I would like to see the test changes I requested, though.

Thanks!

[hawkw]

This is kind of a nitpick, but I feel like DisplayArgs isn't really the clearest name for this --- it sounds like it's specifically referring to fmt::Display, but the struct is also used when the event is recorded using Debug. How about calling it EventArgs, instead, since it configures an event emitted for a return value?

[nitnelave]

Yeah, I didn't have a great idea of how to name it. EventArgs sounds better, thanks.

[hawkw]

style nit: i think we can maybe simplify the nested if chain here a little bit if we wrote something like this:

Suggested change

	} else {
	if result.mode != FormatMode::default() {
	return Err(content.error("expected only a single format argument"));
	}
	let maybe_mode: Option<Ident> = content.parse()?;
	if let Some(ident) = maybe_mode {
	match ident.to_string().as_str() {
	"Debug" => result.mode = FormatMode::Debug,
	"Display" => result.mode = FormatMode::Display,
	_ => {
	return Err(syn::Error::new(
	ident.span(),
	"unknown error mode, must be Debug or Display",
	))
	}
	}
	}
	}
	} else if result.mode != FormatMode::default() {
	return Err(content.error("expected only a single format argument"));
	} else if let Some(maybe_mode) = content.parse::<Option<Ident>>?() {
	match maybe_mode.to_string().as_str() {
	"Debug" => result.mode = FormatMode::Debug,
	"Display" => result.mode = FormatMode::Display,
	_ => return Err(syn::Error::new(
	ident.span(),
	"unknown error mode, must be Debug or Display",
	)),
	}
	}

[nitnelave]

That's less nested, thanks.

[hawkw]

let's assert that the span itself is at the WARN level here:

Suggested change

	let span = span::mock().named("ret_warn_info");
	let span = span::mock().named("ret_warn_info").at_level(Level::WARN);

[nitnelave]

Done.

[hawkw]

and let's assert that the span is at the WARN level here, as well

Suggested change

	let span = span::mock().named("ret_dbg_warn");
	let span = span::mock().named("ret_dbg_warn").at_level(Level::WARN)

[nitnelave]

Also done.

[hawkw]

I'm not a big fan of how we pass in an Option<Level> and a bool that indicates whether the default should be ERROR or not...this feels a little awkward. What if, instead, we changed this so that it's a method on &Level, and changed the code that currently calls this with an &Option<Level> so that it uses Option::unwrap_or with the default value and just calls level.to_tokens() afterwards? This might require a change to how the Level type is represented internally, but it seems like it might be a bit neater...

[nitnelave]

Cleaned up. Now level() returns a Level since it implements ToTokens. For EventArgs, we can pass the default explicitly.

[hawkw]

this isn't a big deal, especially since i know this code was already here, but i wonder if it would be a bit nicer to change this code so that determining which tracing::Level a string or integer value corresponds to occurred in a Parse implementation for Level, rather than in ToTokens?

That way, the error we emit if we got an unexpected level value could happen at parsing-time, when we have access to the span that the level tokens correspond to, rather than quote!ing a compile_error! in the ToTokens impl. That way, I believe the invalid level value would be underlined in the compiler error, which is a bit nicer especially now that we have multiple places where a level might occur in the #[instrument] arguments.

It's fine if you'd rather not address this suggestion right now, since this isn't code that's added in your PR, but it might be worth doing if you're interested. Otherwise, I'm happy to take care of it later.

[nitnelave]

I moved the errors to the Parse implementation, and implemented ToTokens for Level, which leads to a much cleaner interface.

[nitnelave]

Rebased, cleaned up, ready for review.

[nitnelave]

I need a second copy because one gets moved out.

[hawkw]

Overall, the code change looks great now, thanks for taking the time to address my suggestions! I commented on a couple minor style tweaks you may want to make, but none of them are important.

One last thing before we merge this, though: let's make sure the documentation is updated to show how to change the level of an err/ret event. The documentation for ret states that it will always have the same level as the span, here:

tracing/tracing-attributes/src/lib.rs

Lines 230 to 232 in 75b6c30

	/// The return value event will have the same level as the span generated by `#[instrument]`.
	/// By default, this will be `TRACE`, but if the level is overridden, the event will be at the same
	/// level.

so let's make sure to explain how to override it. Similarly, the err docs here should also probably be updated:

tracing/tracing-attributes/src/lib.rs

Lines 249 to 262 in 75b6c30

	/// If the function returns a `Result<T, E>` and `E` implements `std::fmt::Display`, you can add
	/// `err` or `err(Display)` to emit error events when the function returns `Err`:
	///
	/// ```
	/// # use tracing_attributes::instrument;
	/// #[instrument(err)]
	/// fn my_function(arg: usize) -> Result<(), std::io::Error> {
	/// Ok(())
	/// }
	/// ```
	///
	/// By default, error values will be recorded using their `std::fmt::Display` implementations.
	/// If an error implements `std::fmt::Debug`, it can be recorded using its `Debug` implementation
	/// instead, by writing `err(Debug)`:

[hawkw]

CI failure is unrelated to this change, PR #2344 should fix that.

[nitnelave]

One last thing before we merge this, though: let's make sure the documentation is updated

Yeah, actually I thought of it today and it's been bothering me all day :D What do you think of the new changes?

I also rebased on top of the new master, so the CI should work now.

Note: If you want, I can squash my commits into one for a cleaner PR. I keep them separate for now for easier review (you see what changed since your last review).

[hawkw]

One last thing before we merge this, though: let's make sure the documentation is updated

Yeah, actually I thought of it today and it's been bothering me all day :D What do you think of the new changes?

Thanks, I'm about to take a look :)

Note: If you want, I can squash my commits into one for a cleaner PR. I keep them separate for now for easier review (you see what changed since your last review).

We typically merge PRs by squashing anyway, so you don't have to worry about squashing your commits pre-merge. It's generally much nicer for reviewers to see all the changes as separate commits. Thanks for asking!

[hawkw]

looks good to me! i left a couple very minor suggestions on the documentation changes.

[nitnelave]

Committed your suggestions, they were good.

[hawkw]

This looks great! I'm going to merge it now.

Thanks again for working on this!