Can we drop the telemetry for VT sequences? #20011

Closed
opened 2026-01-31 07:00:30 +00:00 by claunia · 4 comments
Owner

Originally created by @j4james on GitHub (May 30, 2023).

Originally assigned to: @j4james on GitHub.

Description of the new feature/enhancement

We've now reached the point that if we add another VT sequence to the telemetry tracker, the TraceLoggingWriteActivity call will fail to compile. I'm not exactly sure what the problem is, but the initial warning/error is: pasting '_tlg_FOR_imp' and '(' does not result in a valid preprocessing token, after which there are another 20 or so lines of errors.

It's possible there is a way to work around that, but if we don't really need this telemetry, it would be nicer if we could just drop it altogether. And maybe that will save a bit of parsing overhead in the process.

Proposed technical implementation details (optional)

Drop the TermTelemetry class, and then remove all code that was making calls to it.

Originally created by @j4james on GitHub (May 30, 2023). Originally assigned to: @j4james on GitHub. # Description of the new feature/enhancement We've now reached the point that if we add another VT sequence to the telemetry tracker, the `TraceLoggingWriteActivity` call will fail to compile. I'm not exactly sure what the problem is, but the initial warning/error is: `pasting '_tlg_FOR_imp' and '(' does not result in a valid preprocessing token`, after which there are another 20 or so lines of errors. It's possible there is a way to work around that, but if we don't really need this telemetry, it would be nicer if we could just drop it altogether. And maybe that will save a bit of parsing overhead in the process. # Proposed technical implementation details (optional) Drop the `TermTelemetry` class, and then remove all code that was making calls to it.
claunia added the Help WantedIssue-TaskIn-PRNeeds-Tag-FixProduct-MetaPriority-2 labels 2026-01-31 07:00:31 +00:00
Author
Owner

@zadjii-msft commented on GitHub (May 30, 2023):

I'm all for it. This was more valuable when we like, hardly implemented any sequences. I don't think this is even hooked up to anything at this point.

@zadjii-msft commented on GitHub (May 30, 2023): I'm all for it. This was more valuable when we like, hardly implemented any sequences. I don't think this is even hooked up to anything at this point.
Author
Owner

@DHowett commented on GitHub (May 30, 2023):

YES. PLEASE. Thank you!

@DHowett commented on GitHub (May 30, 2023): YES. PLEASE. Thank you!
Author
Owner

@DHowett commented on GitHub (May 30, 2023):

The way we implemented this telemetry is wrong, anyway - TraceLogging is supposed to be zero-cost if it is not enabled, so every time we put the log behind a function call we're wasting a branch and likely cache fault to determine we aren't supposed to be doing anything.

@DHowett commented on GitHub (May 30, 2023): The way we implemented this telemetry is wrong, anyway - `TraceLogging` is supposed to be zero-cost if it is not enabled, so every time we put the log behind a function call we're wasting a branch and likely cache fault to determine we aren't supposed to be doing anything.
Author
Owner

@bishalkar10 commented on GitHub (May 30, 2023):

I am very new to programming. I am confused how can I contribute to open source. I am proficient in html, css, javascript and learning react now. Can I contribute somehow?

@bishalkar10 commented on GitHub (May 30, 2023): I am very new to programming. I am confused how can I contribute to open source. I am proficient in html, css, javascript and learning react now. Can I contribute somehow?
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#20011