-
Notifications
You must be signed in to change notification settings - Fork 256
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
An RFC for inline expression and value logging #317
Conversation
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. |
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. |
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 👍 on this or the original comment. If you have any reservations, please leave a comment here outlining your concerns. If we haven't registered any concerns by February 15 then I'll merge this one in and we'll ship it in |
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. |
So can we merge this and move on to considering the implementation PR? 🙈 🤞 |
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. |
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. |
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; |
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 |
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 |
So maybe what we need to do is look at a project like |
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. |
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 |
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. |
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
|
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.");
}
} |
And for that in particular, and for all the other details you are providing, I am forever in your debt! |
@Centril, to continue this 💓fest (or risk a horrible breakup) this works too: panic!(errorv!("fatal attraction💓!")); DEFINATELY NOT in this RFC or its implementation PR, but separately, I foresee the creation of the ellusive (...drum role...) |
Thanks for working through this @dekellum!
Ah that's an interesting point. In the larger Rust codebases I've worked on I've always ended up defining my own logging macros anyways, so it doesn't seem unreasonable to want to do this.
It is a bit of a nuisance, but spun another way it's an encouragement to improve the utility of your log macros by adding a message. If I saw an empty string in a logging expression in a code review I'd probably be a little suspicious.
That's fair! The log macros are already a bit obtuse, but I think that's just how macros end up, so am not too worried about that. Personally, over the long term I can see us completely rebuilding the macros in |
I've updated the RFC to include some additional motivation, and for the design change for the two customization parameters. |
Regarding the design question of -v macro parameter ordering: The macro's can be made to work with either order, so I believe this is purely a question of clarity of design, including least-surprise, as well as long term ease of use. My position remains to stay with the existing design and RFC (before and as recently updated). Here is a more detailed justification: (1) If you consider the optional target, which is first in every public
This would make explaining the parameters that much harder. Or would you prefer, for this alternative, to also move the parameter target to the right? (And if so, where exactly?) Whereas, the current RFC design has the only required parameter on the far right:
(2) Compared with the existing message formatting macros, parameters:
As previously noted, the current -v macro design is more consistent with the existing message formatting macros. This extends to debugv!("refine", some_result); …and converting to: debug!("refine some result: {:?}", some_result); or vice versa. (3) The current argument order matches the output order of the message: debugv!("prefix in code and in output", logged_at_end()); (4) Your prior example, @KodrAus, is at least 101 characters long, so rustfmt would yield: x = infov!(
"noise noise noise noise noise noise noise noise noise noise noise noise",
1 / 2,
); Which doesn't obscure the expression. Also, I suspect that in common usage the prefix context will be much shorter given that the expression is also logged. If it really needs to be this long, it probably makes sense to factor out the logging into a (5) I'm not aware of any existing precedence in Rust for the proposed alternative ordering, i.e. a parameter that will be returned, should be first? I wouldn't be surprised if some other language does have some precedence for some order, but it may or may not be relevant to Rust. |
Hmm, I think we're risking becoming a bit of an echo-chamber on the ordering question. The parameter ordering is starting to seem like the kind of design decision we'd be better off determining through actual usage, right now it's a bit arbitrary and we could justify it in either direction. So based on that I'd be happy to stick with the order you've already specified, but reserve the right to change it based on user feedback before 'stabilization'. Which brings up the question of how we'd like to roll this out. We don't really have a history of unstable features in Any thoughts on that? |
I have re-read the RFC. This is much improved from the original draft -- nicely done. The proposed design has improved and the motivation has been fleshed out. Still I don't think this feature should be accepted. I understand that it is something many people would like to use, and I see that it makes certain code easier to type, and it results in output that is prettier than mixing log+dbg, but I don't think the cost–benefit is where it would need to be for this change to be a net positive on the experience of logging in Rust. MasteryMainly I am concerned that we are adding decisions and knowledge where there didn't used to be decisions and knowledge required. I see a lot of value for new and experienced log users alike from never having to pause and think whether code would be "cleaner" if we logged a warning the other way of logging a warning. I never want there to be a debate on a code review, or even a single comment asking the (probably less experienced) author to please do it the other way. I don't want people to try to articulate elaborate "best practices" for when to logv in expression position, when to logv in statement position, when to normal log that all just boil down to personal preference. I don't want people to stumble on logv performing a move of its arguments when as a beginner we all had to learn as a special case that println (and by extension, log) do not move arguments despite writing no In my view the entire experience of there now being more to learn, more steps before attaining complete mastery of the crate, and more things to think when writing code, make the feature not worth its benefits. I like that the log crate today is easy to master. I find it freeing -- because when you want to log, for the most part you just do it and it's clear how. AnalogyMy position may sound weird because if people want to use a thing and it makes code easier to write and maybe read, is an obvious generalization of existing log and std APIs, and all the other benefits, then of course we should accept it. I think over time I have come to wish for more restraint in the design process than that. I will point out one past design question where I think restraint came up in a way that shares a lot of similarities with this one -- the issue of See the thread, but basically the quote crate supports interpolation of single variables with
that is to say almost every good thing that you could say about a feature, and everything that we traditionally look for when evaluating and accepting features. And yet when I considered the aggregate experience of using the crate, and everything that entails, I felt that the feature would lead to a worse experience for users. In other words it makes the crate worse. I have a very similar feeling on this RFC. I think it is good for so many reasons, and yet altogether it is a bad thing for this crate. CounterproposalWe keep using the existing log macros, we use dbg for ephemeral debugging, we live with the differences in formatting between them, and we fix env_logger (rust-cli/env_logger#127) to behave well under the test harness. |
Yeah I didn't think I'd convince you of anything, @dtolnay! I do appreciate your intuition, but I think it embeds a certain logical fallacy regarding the current state of the ecosystem, circa rust 1.32.
Said another way, your position on this would be much more compelling, to me at least, if Finally, the I want to make it clear, I'm not bashing |
If you are following along… I was finally able to use some of my open source time budget for using
let record = debugv!(reader.read(&tune))?.unwrap(); This Otherwise the changes are pretty boring, actually, which I think is a useful thing to demonstrate with a feature such as this, and the recent suggestion that it could be a distraction, or something worse. There is no currying, or other forms of leftist propaganda in these changes! In this setting, its much more of a 1-to-1 mapping of the intended use for |
My starting point for this RFC was Rust is an expression-oriented language, so it seems reasonable to offer expression-oriented log macros. In that context, having an equivalent set of If we're going to continue down this path then I think we could alleviate some of the possible confusion around whether you should be using |
If the sole purpose of the new macro is for debugging, then one possibility to consider is that If However, one important aspect of this is that |
Related to the feature gate sub-thread, which I still owe you a response to @KodrAus, we could separate @Centril, I would prefer to not, by default, shadow your |
I like the idea to add just log::dbg with the same input patterns as std::dbg. I would expect an RFC with justification explaining the advantages over simply using std::dbg for that use case (given that dbg use cases tend to be about getting the job done, not about beauty of output, so differences in the formatting of the filename/line between log output and dbg output doesn't seem so detrimental). But the idea seems like a promising direction to me! |
@KodrAus wrote:
I've consistently agreed with that, and believe the current RFC is a good expression of, and design for, that goal. Yes
See my response above, but TL;DR: overblown. Could be applied to anything new in the logging space, like for example, Structured Logging. Just to be clear at this point, I'm not going to write a new alternative RFC that dumbs this down to a single
@dtolnay, can you explain how you are somehow more comfortable with this I tried to offer several points of comprise, but honestly even the whole feature gate, non-stable concept is over extending. The -v feature is purely macro-rules, and users that don't want these macros don't have to import them. How much of this conservatism ("more restraint") just stems from this uniquely-rust-inflicted SemVer cul-de-sac that the the |
Selecting features where the argument cannot be applied is not the point. The goal is selecting features whose benefits outweigh their costs. Despite an argument being applicable to a feature, it is possible for that feature to be worthwhile because it brings greater benefits.
None, I think. I'm not sure how that would have anything to do with it. My view continues to be that the RFC would be a net bad thing for this crate. I would hold the same opinion if we were designing the crate from scratch with no versioning considerations. Similar to how I think the benefits of All of these things would have undeniable benefits if added. All of them are things that people would happily use. Any of these, if it gets added, would have people excitedly thinking "finally! I have been waiting to use that". People would use them and love them because they only see the benefits; these features are easy to want. It is much harder in general for people to perceive drawbacks of things like any of these three (logv, #{expr}, ++) because it requires them to think (and have experience thinking) about the entire experience of a feature, rather than individual instances of using it.
match op {
- Op::Write(byte) => {
- outbuf[pos] = byte;
- pos += 1;
- }
+ Op::Write(byte) => outbuf[pos++] = byte, |
@dtolnay: I do hesitate to drag Structured Logging in for example purposes, my apologize @KodrAus , but then again, it at least has to do with logging! If I understand @KodrAus correctly on that feature, he is going to just release a new set of logging macro's in another crate that are structured logging friendly, for example, preserving the message template and named argument as structure. So this just moves the problem up beyond Looks like tokio-trace might be including With your position @dtolnay, are we effectively relegating |
I don't see it playing out that way, no. If it does play out that way, it won't be for lack of logv macros. |
On a re-reading of the thread, I noticed that you conflate the position of being against these new macros as Conservatism is opposition to change or innovation. What I meant by restraint is different: critically evaluating tradeoffs. The example of Accepting features is not a requirement for staying a successful and relevant library long-term. Evaluating tradeoffs is a requirement. |
@dtolnay: Many trade-offs were evaluated in just taking this design and RFC to its current point. You needn't lecture further on that as a concept. Unfortunately, I think you and I are just going to have to agree to disagree on our respective risk vs. benefit analysis of the current RFC. I'm still perplexed and missing an important facet of your position, regarding the above mentioned Sincerely, I thank you for your time and consideration. |
More -v macro usage in a realistic setting: dekellum/body-image@0708dba7a The -v form composes well with futures 0.1 and its typical method chaining, here with some usage of the optional message prefix. Still just in test code, but of course a well tested project will have a lot of test code. |
That's an interesting idea that sounds promising to me too 🤔 If we did something like that it would probably be instead of something like
Hmm, I wouldn't quite go that far. Creating confusion by offering a parallel and not obviously more applicable API is a concern that's tricky to measure and a bit speculative, but it's a valid one nonetheless.
The goal of that work is a little different. It's a point-in-time effort to unblock the benefits for the growing list of structured frameworks to integrate with
I have to disagree with this point. It does seem unnecessary if we think of this API purely in terms of its immediate impact on a codebase that imports
That sounds like a fair starting point to me, but I would put either just Also, I realise we've never explicitly stated what our expectations on RFCs are here in |
So I think the current RFC is a well reasoned and complete design for the expression and value logging feature as conceived. Thanks for all your input helping to make the RFC and design better! This seems now stalled, perhaps awaiting more compelling input, on the existential question: Will users be confused by having the choice of using this? (TL;DR: Ultimately I think that prospective users are smarter than that, and will be well prepared by use of To obtain more feedback, as well as to get on with things, I've gone ahead and implemented your earliest advice, @KodrAus, by publishing the work as the standalone tao-log crate. Editing a project's Cargo.toml to add This is released under the same license as Of course I would also appreciate any feedback you might have on tao-log and its documentation, here or via the dekellum/tao-log repo. For example, its currently unclear if my changes for a new overview of log output macros would be well received here in |
What is holding us from approving this RFC? |
@pravic I am still strongly opposed to accepting this into the crate. See my writeups above, which still reflect my current opinion. The only new information relative to the 2019 discussion is that the proposed macros have been available on crates.io for >2 years (#317 (comment)) and so far nobody outside the RFC author has found it worth using that crate. Probably if that crate were to become somewhat widely used then we could reevaluate here. |
It's been a while since we've visited this. It doesn't look like something we'll accept in I'll go ahead and close this one now to keep our open set of things closer to what's actively on our plates. |
@dtolnay Given that I specifically wanted this feature for the past year and gave a couple of attempts at trying to find it, I don't think this is a valid way to judge if folks are looking for this feature.
That sounds a bit catty... Also wrong... Given that it's got 7K downloads |
Add support to
log
for inline expression and value logging, as a superset of thedbg!
macro recently added to ruststd
.Rendered
Implementation PR: #316