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

DRAFT Spec for Buffer Exporting and Logging #11090

Merged
6 commits merged into from
Dec 13, 2022
Merged

DRAFT Spec for Buffer Exporting and Logging #11090

6 commits merged into from
Dec 13, 2022

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Aug 31, 2021

doc link

Summary of the Pull Request

This is an intentionally brief spec to address the full scope of #642. The
intention of this spec is to quickly build consensus around all the features we
want for logging, and prepare an implementation plan.

Abstract

A common user need is the ability to export the history of a terminal session to
a file, for later inspection or validation. This is something that could be
triggered manually. Many terminal emulators provide the ability to automatically
log the output of a session to a file, so the history is always captured. This
spec will address improvements to the Windows Terminal to enable these kinds of
exporting and logging scenarios.

PR Checklist

Detailed Description of the Pull Request / Additional comments

*** read the spec ***

Open Discussion

  • What formatting string syntax and variables do we want to use?
  • How exactly do we want to handle "log printable output"? Do we include backspaces? Do we only log on newlines?
  • > maybe consider even simpler options like just ${date} and ${time}, and allow for future variants with something like ${date:yyyy-mm-dd} or ${time:hhmm}

@github-actions

This comment has been minimized.

@zadjii-msft zadjii-msft added Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. labels Aug 31, 2021
on close? on newline? It's unclear exactly when PuTTY flushes with this off.
Need more coffee. -->

### Future Considerations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideas/Considerations:

  1. Text formatting
    • Does the output file record the formatting of the text (i.e. color, font, etc.)? Or is the output ".log" file just a txt file?
    • If we already can output formatted text, we should add this as a possible configuration (later imo, just do plain text for now).
    • Maybe we could output to an ".html" file using TextBuffer::GenHTML?
  2. Super logging (no idea what to call this, but it needed a cool name)
    • I remember @DHowett created a cool thing one time where you could record and play back a session in Conhost. I don't remember the specifics. But that seems like something that could be a part of this logging discussion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Nope, it's just text.
  • Cool idea, I'll throw it on Future Considerations
  1. I think you're thinking of asciicast/asciinema which we're tracking in Feature request: Recording #469. I'll link the two up.
  • Dustin's thing recorded raw API calls from the driver. It was awesome, but absolutely overkill for this feature 😄


I'm proposing the following actions and profile settings

* New Action: `exportBuffer()`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* New Action: `exportBuffer()`.
* New Action: `exportBuffer`.

Comment on lines 80 to 81
- `path`: Same as the `path` in the `ExportBufferArgs` above
- `append`: Same as the `append` in the `ExportBufferArgs` above
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `path`: Same as the `path` in the `ExportBufferArgs` above
- `append`: Same as the `append` in the `ExportBufferArgs` above
- `path`: Same as the `path` in the `exportBuffer` above
- `append`: Same as the `append` in the `exportBuffer` above

ExportBufferArgs wasn't introduced... but I guess you also mean to create a mechanism like NewTerminalArgs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yea derp, I just kinda implied that ExportBufferArgs were the ActionArgs for the exportBuffer action.

Comment on lines 82 to 87
- `captureAllOutput`: (boolean, defaults to `false`) When true, don't log only
printable characters, also log non-printable escape characters written to
the Terminal.
- `captureInput`: (boolean, defaults to `false`) Additionally log input to the
Terminal to the file. Input will be formatted as the traditional VT
sequences, rather than the full `win32-input` encoding.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea:

  • remove captureAllOutput and captureInput
  • (names are still a work in progress, but you get the idea) add captureHiddenData: (flag enum { none, vtKeyboardInput, vtMouseInput, vtOutput, all })

- `newFileEveryDay`: (boolean, defaults to `false`) This requires the `day` to
be an element of the path format string. When logging with this setting,
opens a new file at midnight and starts writing that one.
<!-- TODO! - `flushFrequently`: (boolean, defaults to `true`) -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, I found it in the PuTTY docs, thought it was interesting, but commented out because I hadn't investigated it, then forgot to remove the comments before shipping the PR. Will move to future considerations, since it doesn't seem critical to the design.

Comment on lines 213 to 216
My worry with logging the backspaces is that conpty is sometimes a bit noisier
than it needs to be with using `^H` as a cursor positioning sequence. Should we
only log lines when the cursor newlines or otherwise moves from the line it is
currently on?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we should only log when we move to another line. Then just get whatever text was at that line and throw it in a file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens in vim then? Or when performing a cls? I guess the mechanics of this we'll have to compare with PuTTY a bit closer.

Copy link
Contributor

@Don-Vito Don-Vito Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understood the discussion correctly, but we must be able to log everything that happened. Even within the same line, and even if it is noisy. These logs are sometimes required for investigating/debugguing.

Since there might be too much updates we will need to use some async writes with buffering (and ensure everything is flushed when closing).

Probably it should be configured by verbosity level

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my real concern is "does backspace count as a printable character". Yes-ish? By itself it's a non-destructive character. But if we don't treat it as printable, then asdfBkspBksp gets emitted as asdf^h ^h^h ^h, but then would get logged as asdf (with two spaces at the end there). So I guess, backspace should count as printable.

The "log everything" option is frankly just way easier to implement than the "printable characters only" one. Maybe we start with that one 😛

should we log the current buffer contents as well?

I'm inclined to lean towards simply "all future output", and ignore any current
buffer content. If the user rally wants to log the current buffer contents _and_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
buffer content. If the user rally wants to log the current buffer contents _and_
buffer content. If the user really wants to log the current buffer contents _and_

started logging with `toggleLogging`, what should we log? All future output? Or
should we log the current buffer contents as well?

I'm inclined to lean towards simply "all future output", and ignore any current
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree. In my mind, toggleLogging is intended to be "I want to start writing to the file now. Up until this point, I was setting up the terminal to be in a good state".

`exportBuffer` to a file, then `toggleLogging` to that same file with
`"append":true`.

## Potential Issues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideas/Considerations:

  1. How will logging work if you resize during a logging session?

Comment on lines 333 to 336
<!-- * [ ] `LoggingSettings` property for "Flush log frequently", defaults to
`true`(?). This causes us to flush all output to the file, instead of just...
on close? on newline? It's unclear exactly when PuTTY flushes with this off.
Need more coffee. -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get more ☕? haha

@WSLUser
Copy link
Contributor

WSLUser commented Sep 2, 2021

I hope HyperTerminal from XP is also considered as example. I would like to see the same feature is available (for example you can choose when to start and stop the session and the log file is generated in plain text so you can read it in Notepad).

@zadjii-msft
Copy link
Member Author

you can choose when to start and stop the session and the log file is generated in plain text so you can read it in Notepad

Yep, that's already in the spec.

@zadjii-msft zadjii-msft self-assigned this Sep 13, 2021
ghost pushed a commit that referenced this pull request Jan 12, 2022
This adds an action for the context menu entry we added in #11062. That PR added support for exporting the buffer, exclusively through the tab item's context menu. This adds an action that can additionally be bound, which also can export the buffer to a file. This action accepts a `path` param. If empty/ommitted, then the Terminal will prompt for the file to export the buffer to. 

* Does a part of #9700
* Spec in #11090, but I doubt this is contentious
* [x] This will satisfy #12052
* [x] I work here
* [x] docs added: MicrosoftDocs/terminal#479
@zadjii-msft
Copy link
Member Author

I'm officially moving this to the drafts/ folder to clear out the PR backlog. We're not making any progress on this immediately.

@zadjii-msft zadjii-msft removed their assignment Dec 12, 2022
@zadjii-msft zadjii-msft changed the title Spec for Buffer Exporting and Logging DRAFT Spec for Buffer Exporting and Logging Dec 12, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRAFT!!

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 13, 2022
@ghost
Copy link

ghost commented Dec 13, 2022

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 274bbf0 into main Dec 13, 2022
@ghost ghost deleted the dev/migrie/s/642-logging branch December 13, 2022 17:06
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants