Skip to content
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

Improve DebugPrinter #817

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Improve DebugPrinter #817

merged 2 commits into from
Sep 21, 2022

Conversation

rukai
Copy link
Member

@rukai rukai commented Sep 20, 2022

There are two benefits here:

  • users of the DebugPrinter transform are given a much higher level message log.
  • developers get access to much more succinct debug logs with tracing::error!("look at this {}", message.to_high_level_string()). In particular I was overwhelmed by all the log noise when investigating the failure on CassandraSinkCluster: topology task now processes events #812

In order to allow parsing the message I couldnt implement Display for Message due to needing mutable access, so instead I added a to_high_level_string method.

While writing the format I wanted to prioritize both terseness and including every underlying field in some way.
To do this I was willing to introduce ambiguity in many places, but that is ok because its a format for humans to read not computers.
Some instances where this came up is:

  • Options display nothing when None
  • Vecs display nothing when empty
  • cql query is unparsed text instead of AST
  • some fields arent labelled when the meaning should be obvious (assuming you are already familiar with the field)

The result metadata types formatting are not great right now but I want to refactor the underlying types there sometime so not too fussed about that atm.

Many message types are just stubbed to give the standard "{:?}" debug output, I expect we will want to come back and implement those properly later on.

Here is some before and afters:

system.local query/response before:
image

system.local query/response after:
image

And a bonus one, that demonstrates multi-row results that I couldnt find a before screenshot for:
image

@conorbros
Copy link
Member

Just a thought as I'd be pretty happy to go ahead with this as is because it's very useful:

Could we do a parameter called expand_long_lists: bool or similar? If false then large lists like the tokens would be truncated to only a couple of elements seeing as it's unlikely you'd want to inspect the entire token list very often.

@rukai
Copy link
Member Author

rukai commented Sep 20, 2022

It sounds useful for debugging, however:

  • I dont think we would want to expose it in DebugPrinter
  • trying to come up with an API that is flexible while keeping the implementation and API simple is going to be challenging.
  • it takes some foresight to know the cases where we truly only want to see part of the message.
    I can imagine it would be annoying to be reading through a debug log and suddenly finding that we need to rerun the test without the truncation because we guessed wrong.

Possibly the way to do it would be to have a configurable line length limit and truncate any lines that go over that limit.

I would like to later on change Varchar("blah") to format as just "blah", that should reduce the result noise without resorting to truncation.
Although I am a little apprehensive about that approach due to the NO_METADATA flag option.

Even with that all said, I wouldnt object if you made a follow up PR to introduce such functionality.

@conorbros conorbros enabled auto-merge (squash) September 21, 2022 03:40
@conorbros conorbros merged commit 9d5b50c into shotover:main Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants