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

[Dev] Fixed name of logging method in managed logging #35952

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

drawbyperpetual
Copy link
Collaborator

Summary of the Pull Request

The name of the logging method is included in logs. Sometimes the name of the method can be:

  • Wrong due to method-inlining invalidating assumptions made within logging routines.
  • Mangled for Async methods.

This PR attempts to fix these issues.

PR Checklist

  • Closes: #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated
  • New binaries: Added on the required places
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Issue 1

Apparently due to inlining (no other plausible explanation comes to mind), we sometimes log the incorrect method name:

[23:18:41.8536506] [Trace] AsyncMethodBuilderCore::Start
[23:18:52.7273013] [Debug] AsyncStateMachineBox`1::ExecutionContextCallback

This has been fixed by marking logging methods with NoInlining so we can use a consistent frame index on the call-stack to identify the actual logging method.

Issue 2

Async methods are logged with a mangled name because of compiler-generated code for the async state-machine:

[23:18:04.7979690] [Trace] <ToMarkdownAsync>d__0::MoveNext
[23:18:52.7053439] [Trace] <TryCopyPasteAsync>d__0::MoveNext

This has been fixed by detecting this scenario and attempting to correct it by looking up the call-stack.

Validation Steps Performed

Ran a number of PowerToys modules in Release and Debug and verified method names were logged correctly.

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants