Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAn RFC for inline expression and value logging #317
Conversation
dekellum
referenced this pull request
Jan 30, 2019
Open
Add pub "-v" macros for inline expression logging #316
This comment has been minimized.
This comment has been minimized.
Thanks for putting this together @dekellum! I'll have a proper read shortly, but I've posted a link to the subreddit that points back here, just in case we get any comments there. |
KodrAus
added
the
rfc
label
Jan 30, 2019
KodrAus
reviewed
Jan 30, 2019
tracev!("{} = {:x}", i); // hexadecimal format value | ||
tracev!("{} = {:#?}", i); // use pretty, multi-line format | ||
let rem = infov!("{1:?} remaining ({0})", deadline - Instance::now()); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
KodrAus
Feb 17, 2019
•
Contributor
Thinking about real-world examples more, I'm guessing these macros would be useful as a way to 'annotate' an expression to trace your way through interesting operations. That suggests we might actually want the expression and format to be reversed in the macro so the most relevant info when scanning the code appears first:
let rem = deadline - Instance::now();
becomes:
let rem = tracev!(deadline - Instance::now(), "{value:?} remaining ({expr})");
where we could interpolate the expression and value as named args into the format.
This comment has been minimized.
This comment has been minimized.
dekellum
Feb 18, 2019
Author
Contributor
I actually had them named in my implementation branch at one point. It's easy to do. As you likely know, you can still use the position references, "{1} {0}"
even with named arguments, so I could resurect that in the implementation or you could add it later.
This comment has been minimized.
This comment has been minimized.
dekellum
Feb 18, 2019
•
Author
Contributor
Also, in my own limited experimentation I like using "{0} -> {:?}" as the format string and I wonder if that would be a better default format, as "->" doesn't appear in any expressions I'm aware of, but =
can of course be used for assignment in an expression position.
Edit: In other words by using ->
we can avoid more collisions between expression and format.
Thoughts?
Centril
reviewed
Jan 30, 2019
dekellum
added some commits
Jan 31, 2019
This comment has been minimized.
This comment has been minimized.
cc @rust-lang-nursery/libs (apologies for the ping) does anyone have any thoughts on adding As a rough FCP, I'd be on-board with merging and supporting this. |
This comment has been minimized.
This comment has been minimized.
Alrighty, let's try a bit of a community final comment period. I'm on board with this feature as proposed. If you'd also like it, please drop a If we haven't registered any concerns by February 15 then I'll merge this one in and we'll ship it in |
This comment has been minimized.
This comment has been minimized.
ssokolow
commented
Feb 8, 2019
I like what I see in the RFC but I want to qualify that with a caution that I've had far too many "Oh, yeah. I never considered that issue." moments while reading through other, more heavily commented-on RFCs. |
This comment has been minimized.
This comment has been minimized.
So can we merge this and move on to considering the implementation PR? |
This comment has been minimized.
This comment has been minimized.
Thanks for the ping @KodrAus. Looks like I missed your deadline but my reaction anyway: I don't think this RFC is sufficiently motivated as written. The std dbg macro, as explained in its documentation that is also quoted in this RFC, is intended for ephemeral debugging use cases. The thing about ephemeral debugging is that being easy to type and untype is valued above almost everything else. It seems to me like this RFC is trying to use easy-to-type as the primary motivation for a use case other than ephemeral debugging, which seems misconceived. If a log line, even at the debug or trace level, is going to exist for longer than one ephemeral debugging session, I think you will find that it is practically always possible and worthwhile to put in an extra 10 seconds of effort to add more context than just the name of a variable or stringify of an expression. At that point I would find it clearer for the log to occur in a statement by itself rather than being dbg-style nested inside a bigger expression or used as a return value. I have not been involved in API design of the log crate so far so this is just my opinion as an outsider, but I would currently consider myself quite opposed to this change. One thing that might change my opinion is if @dekellum or anyone else could share a single ironclad real world use case, maybe an extract of ~15 lines of code for context, in which they believe one of these new macros is the best and clearest way to express what the code needs to do. In other words show me the perfect use case for any one of these macros. I don't see that in the RFC text; all the code seems to be toy examples intended to demonstrate syntax only, not intended to justify why the new macros improve on the status quo. |
This comment has been minimized.
This comment has been minimized.
Centril
commented
Feb 17, 2019
If we take an application such as rfcbot there are some places where Getting the value you gave back is also nice since it doesn't force you to make temporaries just for the purpose of logging. |
This comment has been minimized.
This comment has been minimized.
File and line number are already present in the log record (https://docs.rs/log/0.4.6/log/struct.Record.html). It sounds like rfcbot will want to configure its logger to include that file and line number in its output. As far as I can tell from the RFC and the implementation PR, this RFC does not propose doing anything different with file and line information than any of the existing log macros. Without using a logger that emits file and line in the output, rfcbot would continue not having file and line in its log output even if it switches to use logv macros. Sorry if I missed your point -- my interpretation of your response does not seem related to my concerns.
This is the easy-to-type argument that I explained I find not at all compelling. To be more explicit, my perspective is that: // I would never use this because file + line + "n / 2" is not sufficient
// context to make this log line meaningful to me long in the future.
let m = debugv!(n / 2) - 1;
// I would never use this because it would be more clearly expressed with
// the log being its own statement, despite taking more lines.
let m = debugv!("context context {} = {:?}", n / 2) - 1; |
This comment has been minimized.
This comment has been minimized.
Centril
commented
Feb 17, 2019
Oh... right; then my point is wrong... Thanks for the correction!
From my perspective, it's not really about being more easy-to-type but rather about having to invent temporary names for things that aren't semantically significant; temporary |
This comment has been minimized.
This comment has been minimized.
That date was just something I pulled out of thin air to try make things a bit more tangible. I think it's worth working through your feedback first @dtolnay.
That's a fair point, the main motivation for this is as sugar for capturing the evaluation of some expression. I've been thinking of it as just an alternative way to write log statements that appeals more to some people. As for not using a raw |
This comment has been minimized.
This comment has been minimized.
So maybe what we need to do is look at a project like |
This comment has been minimized.
This comment has been minimized.
I would like to wait for @dekellum to share a use case that they feel perfectly calls for one of these new macros (and include it in the RFC text). I think that is the crucial missing piece before we can form a meaningful shared opinion on this RFC.
This doesn't make sense to me because if something is not semantically significant then it shouldn't be in log messages. If something is semantically significant then it is possible to pick a variable name as well as describe in words in the log message what significance the logged value holds. I think your argument would be easier to articulate and evaluate in the context of a concrete snippet of real world code, which is why I am still pushing for that.
I acknowledge that rightward drift is a thing. In the absence of a concrete use case, I don't acknowledge that doing more things in the same expression solely to avoid indentation is going to result in the clearest solution. I think this argument too would be easier to evaluate in the context of a real world code snippet.
I used the word context but didn't explain what I mean. I don't mean context as in other values in scope (like what file path we tried to open that failed, or what size of buffer is being used, etc). I mean human language that explains why some value appears in my log. For example this pair of log lines in rfcbot: [1] [2]. Which would you rather see a year after writing the code?
You need context to know what is going on with a value that makes it semantically significant (in @Centril's phrasing). Usually this means a verb. In fact every log message in that file uses a verb as context.
I think the RFC needs to include a snippet of real world code that illustrates one of the macros being used in an ideal way -- where the author believes it to be the best solution. Code from rfcbot would meet my bar for real world code. |
This comment has been minimized.
This comment has been minimized.
Right, I guess I've been thinking of this with post-structured-logging blinkers, where you'd write: let command_name = debugv!("Command is to resolve a concern.", get_concern_name()); and The output of final records not being as useful in the |
This comment has been minimized.
This comment has been minimized.
I don't see this discussion being productive until someone has proposed a single actual use of any one of these macros. I am unsubscribing for now, but kindly ping me again when anybody has demonstrated a concrete use case that they believe is a good fit -- or go ahead with the RFC if the consensus among others is that hypothetical use cases are sufficient motivation. I am looking for a single use case that you couldn't write more clearly some other way without the new macros, such as in the case of @KodrAus's last comment which would be more clearly written as: let concern_name = get_concern_name();
debug!("...", concern_name); I would insist on the use case being drawn from real world code as opposed to hypothetical or contrived or abstract, and with enough surrounding code to evaluate whether the proposed macros are the most appropriate solution. |
This comment has been minimized.
This comment has been minimized.
I'm not sure I can offer what you are asking for, or otherwise convince you, @dtolnay. The proposed feature offers a few relatively minor conveniences and is subject, as has been suggested, to style preference. Obviously, every character of log message output can be also be achieved with the existing macros in statement position. I won't partake in re-litigating the This is my limited, real code experience on the matter:
An excerpt of a debugging session with
As this real world example shows, it can actually be useful to use verbatim, a rust code expression, My first reaction when I saw the rust RFCs was: With the @dtolnay, in initial comment:
This exposes a weakness in my terse RFC motivation section, but not the feature. Its perfectly appropriate to add, then shortly remove, uses of the tracev!(some_long_function_call(x, y, z));
\\-- trace log message: some_long_function_call(x, y, z) = ()
\\ or...
tracev!(some_long_function_call(x, y, z));
\\-- trace log message: some_long_function_call(x, y, z) = "return value lost"
\\ or...
let r = tracev!(some_long_function_call(x, y, z));
\\-- trace log message: some_long_function_call(x, y, z) = "return value bound"
If your world view is that each log line (
How I intend to use it, and what I think will practically happen, with this feature in place, while developing and debugging code, is:
(Am I the only one who uses the same process above, with the existing
Sounds like a general procedural vs functional programming style argument? Some will certainly like to use Q: So
|
This comment has been minimized.
This comment has been minimized.
Centril
commented
Feb 18, 2019
The match 1 {
x if dbg!(x) == 1 => {},
_ => {}
}
fn factorial(n: u32) -> u32 {
if dbg!(n <= 1) {
dbg!(1)
} else {
dbg!(n * factorial(n - 1))
}
} You can get back the original
Integration with
Easy... it takes more time to configure
Haskell has both As for real code, having the features here available, I think I would rewrite: // we want to panic if we're unable to find any of the usernames
let parsed_teams = teams::SETUP.team_labels().collect::<Vec<_>>();
info!("parsed teams: {:?}", parsed_teams); as: // we want to panic if we're unable to find any of the usernames
let parsed_teams = infov!(teams::SETUP.team_labels().collect::<Vec<_>>()); or: // we want to panic if we're unable to find any of the usernames
let parsed_teams = teams::SETUP.team_labels().collect::<Vec<_>>();
infov!(parsed_teams); I would rewrite: match Pool::builder()
.max_size(CONFIG.db_pool_size)
.build(manager)
{
Ok(p) => {
info!("DB connection pool established.");
p
},
Err(why) => {
error!("Failed to establish DB connection pool: {}", why);
panic!("Error creating connection pool.");
}
} as: match Pool::builder()
.max_size(CONFIG.db_pool_size)
.build(manager)
{
Ok(p) => infov!(p, "DB connection pool established, {:?}."),
Err(why) => {
error!("Failed to establish DB connection pool: {}", why);
panic!("Error creating connection pool.");
}
} |
This comment has been minimized.
This comment has been minimized.
And for that in particular, and for all the other details you are providing, I am forever in your debt! |
This comment has been minimized.
This comment has been minimized.
@Centril, to continue this panic!(errorv!("fatal attraction💓!")); DEFINATELY NOT in this RFC or its implementation PR, but separately, I foresee the creation of the ellusive (...drum role...) |
This comment has been minimized.
This comment has been minimized.
In this latest round of examples, we've been a bit fast-and-loose with whether or not the expression and value are actually interpolated into the format. Using the match Pool::builder()
.max_size(CONFIG.db_pool_size)
.build(manager)
{
Ok(p) => infov!(p, "DB connection pool established."),
Err(why) => panic!(errorv!(why, "Failed to establish DB connection pool")),
} where format_args!("{msg} ({fmt})", msg = "DB connection pool established", fmt = format_args!("{expr} -> {value:?}", expr = "p", value = p)); with an optional extra argument for the expression format: infov!(p, "DB connection pool established.", "`{expr}` produced `{value:?}`"); If you just call |
This comment has been minimized.
This comment has been minimized.
Interesting idea. In my last half joking case, that's why I suggested/threatened to propose a new If there was a way to make that just work, that would certainly be a plus. Edit: Well there is a hackish way, using zero width format specifier. In Having waited a while for this here, I don't think we should wait for a In your proposed alternative, since both prefix and format are optional, wouldn't we need a marker token like '=', ';', "prefix:", "format:" or the like to distinguish them? But If the normal case doesn't need it, it might not be too bad.. There is already the precedence of the "target:" marker. |
This comment has been minimized.
This comment has been minimized.
I would suggest we simply require a textual message be given if you also want to override the format for the expression and value. |
This comment has been minimized.
This comment has been minimized.
But |
This comment has been minimized.
This comment has been minimized.
Hmm, I wouldn't rely on a Having separate parameters for a message and format at least gives us some clear indication that there's a separate magic part of the format that uses format arguments you aren't explicitly providing. |
This comment has been minimized.
This comment has been minimized.
Also, just to use a trick I just learned from @Centril, this works as intended: let panic_msg = format!("fatal attraction {}!", "💓");
panic!(errorv!(panic_msg)); Edit: But just to be clear, below is probably the better code for this case (well at least, before there is a let panic_msg = format!("fatal attraction {}!", "💓");
error!("{}", panic_msg);
panic!(panic_msg); Edit 2: Added the dumb "{}" pattern, to get it to compile, so sad. |
This comment has been minimized.
This comment has been minimized.
@dekellum splitting the message into text and a format lets you write: infov(p, "A message. We don't need to include any format args"); and guarantee that it will work on the compiler version that
It reduces the barriers to associating a useful message with a |
This comment has been minimized.
This comment has been minimized.
OK, now I understand your position. I'd like to think about that more, but looks like it at least deserves a place in the alternatives of the RFC. |
This comment has been minimized.
This comment has been minimized.
Note, also in the RFC, the order of parameters is currently reversed. To change that I would like to hear a detailed justification. My justification for current order is that it is consistent with the existing message logging (non-v) macros. |
This comment has been minimized.
This comment has been minimized.
My reasoning for reversing the order is that when scanning the code visually it keeps the actual expression on the left where it's closer to the assignment: let x = infov!(1 / 2, "noise noise noise noise noise noise noise noise noise noise noise noise"); vs: let x = infov!("noise noise noise noise noise noise noise noise noise noise noise noise", 1 / 2); I don't necessarily think inconsistency with ordering for the message logging macros is a big deal here. For the message logging macros, the message itself is the significant part of the statement, so coming before args makes sense. For expression logging macros, the expression itself is the significant part of the statement. |
dekellum commentedJan 30, 2019
Add support to
log
for inline expression and value logging, as a superset of thedbg!
macro recently added to ruststd
.Rendered
Implementation PR: #316