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

Add logging for PowerRename #14249

Merged
merged 3 commits into from
Nov 8, 2021
Merged

Add logging for PowerRename #14249

merged 3 commits into from
Nov 8, 2021

Conversation

stefansjfw
Copy link
Collaborator

Move call tracer to common/utils/logger
Add logging to both PowerRename dll and PowerRenameUIHost
Add PowerRename to BugReportTool event viewer collection

Summary of the Pull Request

What is this about:
Logging general PowerRename logic. Not logging any file names to avoid collecting sensitive user data.

What is include in the PR:

How does someone test / validate:

Quality Checklist

  • Linked issue: [Bug report] Logging for PowerRename #14181
  • Communication: I've discussed this with core contributors in the issue.
  • Tests: Added/updated and all pass
  • Installer: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Docs: Added/ updated
  • Binaries: Any new files are added to WXS / YML

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

Move call tracer to common/utils/logger
Add logging to both PowerRename dll and PowerRenameUIHost
Add PowerRename to BugReportTool event viewer collection
Copy link
Collaborator

@SeraphimaZykova SeraphimaZykova left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

@stefansjfw
There is another list for bugreport on compat flags method. Dont ask me how it's called.

See #11561.


@jaimecbernardo
Can you plz block merging for this PR. Thanks.

@htcfreek
Copy link
Collaborator

htcfreek commented Nov 3, 2021

@stefansjfw
Are unhandled exceptions in code/execution logged too?

@stefansjfw
Copy link
Collaborator Author

stefansjfw commented Nov 4, 2021

@stefansjfw There is another list for bugreport on compat flags method. Dont ask me how it's called.

See #11561.

@jaimecbernardo Can you plz block merging for this PR. Thanks.

@htcfreek Let's do this one first then:
#14269

Are unhandled exceptions in code/execution logged too?

I'll go through code and check out what could throw and add logs

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Blocking merge on request.
I think requesting changes will block it.

@htcfreek
Copy link
Collaborator

htcfreek commented Nov 5, 2021

Now that #14296 is merged we can fix conflicts here, review it and unblock/merge this PR.

@stefansjfw
Copy link
Collaborator Author

Now that #14296 is merged we can fix conflicts here, review it and unblock/merge this PR.

Done. Let's do another round of review before merging.

@htcfreek
Copy link
Collaborator

htcfreek commented Nov 6, 2021

Can try it tomorrow. Maybe I get logs for #14180.

@stefansjfw
Copy link
Collaborator Author

Can try it tomorrow. Maybe I get logs for #14180.

That would be great, as I can't repro that one. Thanks!

@htcfreek
Copy link
Collaborator

htcfreek commented Nov 6, 2021

Can try it tomorrow. Maybe I get logs for #14180.

That would be great, as I can't repro that one. Thanks!

Are there any dev builds I can install without needing to build the installer?

@stefansjfw
Copy link
Collaborator Author

stefansjfw commented Nov 6, 2021

Can try it tomorrow. Maybe I get logs for #14180.

That would be great, as I can't repro that one. Thanks!

Are there any dev builds I can install without needing to build the installer?

No, but you can run PowerRename directly by setting PowerRenameUIHost as a startup project. If you want to include items to rename , just add path here:
https://github.com/microsoft/PowerToys/blob/main/src/modules/powerrename/PowerRenameUIHost/PowerRenameUIHost.cpp#L55

@htcfreek
Copy link
Collaborator

htcfreek commented Nov 7, 2021

@stefansjfw
The log is correctly saved to the log folder.
There is a not logged crash. I could reproduce the crash in #14180 and there is no exception in log file.

@stefansjfw
Copy link
Collaborator Author

@stefansjfw The log is correctly saved to the log folder. There is a not logged crash. I could reproduce the crash in #14180 and there is no exception in log file.

Well, I added exception logging where I was expecting them.. :) This shouldn't have happened there,

@htcfreek
Copy link
Collaborator

htcfreek commented Nov 7, 2021

@stefansjfw The log is correctly saved to the log folder. There is a not logged crash. I could reproduce the crash in #14180 and there is no exception in log file.

Well, I added exception logging where I was expecting them.. :) This shouldn't have happened there,

I am okay with what we have.

Copy link
Contributor

@yuyoyuppe yuyoyuppe left a comment

Choose a reason for hiding this comment

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

Code looks good to me

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Unblocking. Rubberstamp LGTM

@stefansjfw stefansjfw merged commit 079a3b4 into main Nov 8, 2021
@stefansjfw stefansjfw deleted the stefan/powerrename_logging branch November 8, 2021 12:02
@Jay-o-Way Jay-o-Way added the Area-Logging Issues regarding the PowerToys logging facility label Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Logging Issues regarding the PowerToys logging facility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants