-
Notifications
You must be signed in to change notification settings - Fork 21
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
Separating conserve library and utility functionality #173
Conversation
tracing
for logging and implement file logging
@sourcefrog any thoughts on this one? |
Hi, yeah, I had not realized you were going to start right away. I can give you some guidance on the weekend. |
Ahh yeah all right. After some fiddling I came up with the concept of extracting the whole logging thing from the library and move it into the binary/into the responsibility of the library users. The prevents conflicts with the use of any other logging systems along with other libraries. My approach would be passing some kind of statistic callbacks for progress logging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without going line by line here are a few thoughts:
- Please don't delete
-D
, I use it. - This would be easier to review as a few PRs, but I understand how you might need to write the whole thing first to understand the code or what you want to change. e.g. introducing better structured error codes could be its own PR.
- Converting messages to
info!
is probably OK in general, but as I mentioned on the bug I'm not sure it's a good idea for e.g. file listings that might be consumed by other programs, orshow_versions
. Or for example json will not be useful if it's mixed with log prefixes. - The
src/bin
entry point should remain small and should configure things in the library. I don't have a goal to migrate lots of code into the binary, rather the reverse. But, making the library configurable by different binaries that e.g. don't want terminal output, would be good.
Yes, perspective changed a bit.
I skipped answering that point since I agree on that one as well, this is still one of my concerns.
Right not I'm not feeling well about any of these solutions.
Okey, I'll keep that in mind. Edit:
|
I'm happy to talk about it more when you want! I would suggest perhaps starting with options to write trace to a log file that is not stdout, without removing |
I'd thought about the system in the recent time and think, I came up with a pretty need system. Using PS: Don't worry, you'll get your |
Thinking about this a bit more overnight: perhaps the monitor interface should look more like something that just accepts Then, the implementation used in testing can simply record the events/problems into a vec to inspect later. A different implementation can report significant events to log events. Another monitor (or perhaps the same one with a flag set) can update nutmeg models. pub enum Event {
StartListBlocks,
ListBlocks { block_count: usize },
FinishListBlocks { block_count: usize },
StartValidateBand { band_id: BandId },
}
pub enum Problem {
...
}
trait Monitor {
fn report_event(&self, event: &Event);
} This would still have, to some extent, parallel state machines in the application code and the monitor. But, the states are not really that complex: validate just proceeds through a few phases. So that would be fine. |
I had the same thought at the beginning, but didn't go for it. When using an enum to propagate all status events we could just pass a callback functions which takes one argument (the In terms of testing, your right. |
The more I think about it, the satisfied am I with the amount of callback methods currently supplied. |
Hey, I intuitively used separate functions since I mainly use that kind of pattern when the state of a struct needs to be observed. Adapting this approach for events, especially if these happen in a predefined order works but might not be the best. Therefore I'll adjust the monitor style accordingly. |
@sourcefrog any notes on the changes made? |
Hi, I appreciate all the work you put into it. At the moment this gets only a small amount of time from me because it's a spare time project and I'm trying to finish cargo-mutants on my weekends. I'm not sure about merging it just as is. It is very big. It does at least indicate some interesting directions. I think the proliferation of monitor methods, the handling of bulk output, and the particular way that trace interacts with the nutmeg view is not quite what I'm comfortable with at the moment. |
Hi, so I finally came back to this over the break to try to work out how to merge or otherwise resolve it. I think overall this is a really good step. The monitor seems fine as it is. The main thing I'd want to change is to have a concept of output to stdout that is not a log message: listings whether json or text of files, versions, etc. I'll try to build on this to put that back in... |
If I' not mistaken you're referring to "show_index_json" within the "show" command? PS: I've updated the PRs description and tile to a more (imo) accurate one as it has quite changed over time. |
Attention: Disabled test 'long_listing_old_archive' as it will currently not compiling as it is
What might work for this kind of case:
Both of these could then be extended:
|
It occurs to me that it might be possible to separate out just the addition of the monitor concept from everything else. That would, at least, get the PR size down a bit, be a step forward itself, and also probably be possible to land without regressing any behavior by introducing log prefixes on bulk output and without needing to update so many tests... |
Well, along with the monitor come the logging/ui changes. A core part of the new monitor system is to separate the whole display part. Therefore we could not just leave the old UI stuff within the conserve library. A possible solution tough would be to replace the logging done via tracing within the binary by just |
08d26ce has a work-in-progress experiment with using the monitor pattern only for validate, including collecting a structured list of problems, which will make testing validation/corruption better.
I'm not sure I'm following here. I agree with using |
I decided to instead merge #197 which builds on these ideas. Thanks for the thoughts and provocation! |
Aim
The aim is to separate library and the actual conserve command line utility.
Currently, progress output was vastly achieved with the conserve library therefore prevent other uses except for the conserve command line utility. The goal is provide a simple but yet powerful developer interface to interfere and monitor conserve process state.
Due to the initial aim, to provide a better way of logging this PR archives two things:
Attention:
This PR submits API breaking changes!
Providing a more powerful developer interface
Tapping into library functions as well as observe the current function progress has been achieved by so called
Monitors
.These monitors can be passed as the last function argument. Depending on the function the monitor receives general progress notifications or other kind of events.
Implementing a logging system conserve utility
The logging system has been based on the
tracing
logging system and includes support for a logfile (fixing #104).The actual output should not differ from the previous conserve utility output.