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

Remove conhost telemetry #16253

Merged
merged 2 commits into from Nov 6, 2023
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Nov 2, 2023

The Telemetry class was implemented as a singleton which stood in
my long-term goal to remove all global variables from the project.
Most telemetry captured by it hasn't been looked at for a long time
and just as much is now pointless (e.g.,_fCtrlPgUpPgDnUsed).
This removes the code.

Validation Steps Performed

  • Still compiles ✅

@lhecker lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Nov 2, 2023
src/host/telemetry.hpp Show resolved Hide resolved
@@ -428,11 +428,6 @@ HRESULT ConsoleCreateIoThread(_In_ HANDLE Server,
[[maybe_unused]] PCONSOLE_API_MSG connectMessage)
try
{
// Create a telemetry instance here - this singleton is responsible for
// setting up the g_hConhostV2EventTraceProvider, which is otherwise not
Copy link
Member

Choose a reason for hiding this comment

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

who sets this up now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

So... about that TraceLoggingRegister call...
mhm

Copy link
Member

Choose a reason for hiding this comment

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

yup!

How did you notice?

break;
case CONSOLE_REAL_UNICODE:
case CONSOLE_FALSE_UNICODE:
Telemetry::Instance().LogApiCall(Telemetry::ApiCall::ReadConsoleOutputCharacter, true);
break;
}

Copy link
Member

Choose a reason for hiding this comment

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

wait somebody's gonna find this in the future and choke on their Soylent

switch (a->StringType)
{
	case 1:
		break;
	case 2:
		break;
	case 3:
		break;
}

complicated no-op. it keeps our enemies off our trail

@@ -428,14 +419,11 @@ static DWORD TraceGetThreadId(CONSOLE_API_MSG* const m)
switch (a->ElementType)
{
case CONSOLE_ATTRIBUTE:
Copy link
Member

Choose a reason for hiding this comment

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

same as below

}
else
{
Telemetry::Instance().LogApiCall(Telemetry::ApiCall::ReadConsoleInput, a->Unicode);
}
Copy link
Member

Choose a reason for hiding this comment

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

rolling on the floor at this point

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 2, 2023
@zadjii-msft zadjii-msft enabled auto-merge (squash) November 6, 2023 21:30
@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 6, 2023
@zadjii-msft zadjii-msft merged commit 17cc109 into main Nov 6, 2023
16 of 17 checks passed
@zadjii-msft zadjii-msft deleted the dev/lhecker/conhost-remove-singletons branch November 6, 2023 22:00
radu-cernatescu pushed a commit to radu-cernatescu/terminal that referenced this pull request Nov 8, 2023
The `Telemetry` class was implemented as a singleton which stood in
my long-term goal to remove all global variables from the project.
Most telemetry captured by it hasn't been looked at for a long time
and just as much is now pointless (e.g.,`_fCtrlPgUpPgDnUsed`).
This removes the code.

## Validation Steps Performed
* Still compiles ✅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants