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.

Sign up for GitHub

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

Jump to bottom

Data diagnostics opt-in #34078

Merged
merged 44 commits into from
Oct 24, 2024
Merged

Data diagnostics opt-in #34078

merged 44 commits into from
Oct 24, 2024

Conversation

stefansjfw
Copy link
Collaborator

@stefansjfw stefansjfw commented Jul 31, 2024 •
edited
Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@stefansjfw
Copy link
Collaborator Author

\azp run

@stefansjfw
Copy link
Collaborator Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

htcfreek
htcfreek suggested changes Jul 31, 2024
View reviewed changes
src/gpo/assets/PowerToys.admx Outdated Show resolved Hide resolved
src/gpo/assets/en-US/PowerToys.adml Outdated Show resolved Hide resolved
src/gpo/assets/en-US/PowerToys.adml Show resolved Hide resolved
@htcfreek
Copy link
Collaborator

htcfreek commented Aug 1, 2024 •
edited
Loading

Existing issue is #964 and we could think about using the existing system gpo instead of creating our own.

@htcfreek htcfreek mentioned this pull request Aug 1, 2024
Open
11 tasks
@microsoft microsoft deleted a comment from github-actions bot Aug 1, 2024
@stefansjfw stefansjfw changed the title [WIP] Data diagnostics opt-in Data diagnostics opt-in Aug 1, 2024
@stefansjfw stefansjfw changed the title Data diagnostics opt-in [WIP] Data diagnostics opt-in Aug 5, 2024
@stefansjfw stefansjfw mentioned this pull request Aug 7, 2024
Open
14 tasks
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@stefansjfw stefansjfw force-pushed the stefan/opt_in_telem branch 6 times, most recently from 7a9b429 to 726b012 Compare August 13, 2024 11:12
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 13, 2024
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

check-spelling found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
jaimecbernardo
jaimecbernardo requested changes Oct 23, 2024
View reviewed changes
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.

I've ran another session of deep testing and it's much better now! Events less spammy in etw folder as well. Great work!
Here's my new feedback:

  • Remove etw folder from BugReport.
  • When we disable Diagnostic data, disable Enable Viewing control as well, it makes no sense for it to be able to change, but allow the .xml data generation button to still be clicked.
  • When we disable Diagnostic data, disable Enable Viewing setting as well. If we leave it enabled, it'll make it so that the next time Diagnostic data is enabled, telemetry will be sent but .etl files won't be saved. Make the expander always opened so the user can see this change.
  • On OOBE Welcome screen, the Diagnostic Data toggle is out of scroll view. Can we make the screen more compact so that's visible? Or perhaps we need to increase the windows default height as well.
  • A link to Settings General screen from OOBE welcome saying "View more diagnostic data options in Settings" might be good as well.
  • I see no events saved from activating PowerRename/FileLocksmith/ImageResizer from tier 1 context menus.
  • I see no events saved from MeasureToolUI as well.
  • PowerToys.Hosts.exe is hanging for me after closing. I launch as admin and press Quit at the first screen.

@stefansjfw
Copy link
Collaborator Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jaimecbernardo
jaimecbernardo approved these changes Oct 24, 2024
View reviewed changes
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.

LGTM! Thank you. Only thing missing before merging is making sure link for info goes somewhere.

@stefansjfw
Copy link
Collaborator Author

@stefansjfw This PR conflicts with #33703. As I like to get #33703 in soon how would you like to work around?

  1. I make my PR ready and you updated the gpo information bar in your PR with the new icon.
  2. We make both ready as is and fix the icon later.
  3. I hold my PR until yours is merged.

I personally prefer option 1.

Sorry for the late reply. This is high priority for current release so let's merge this one first please. Thanks for bringing this up btw.

@htcfreek
Copy link
Collaborator

@stefansjfw This PR conflicts with #33703. As I like to get #33703 in soon how would you like to work around?

  1. I make my PR ready and you updated the gpo information bar in your PR with the new icon.
  2. We make both ready as is and fix the icon later.
  3. I hold my PR until yours is merged.

I personally prefer option 1.

Sorry for the late reply. This is high priority for current release so let's merge this one first please. Thanks for bringing this up btw.

I know. And yet this order makes most sense now. I will update #33703 later.

@stefansjfw stefansjfw changed the title [WIP] Data diagnostics opt-in Data diagnostics opt-in Oct 24, 2024
@jaimecbernardo
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@crutkas
Copy link
Member

crutkas commented Oct 24, 2024

🔥🔥🔥

@jaimecbernardo jaimecbernardo merged commit 133aa85 into main Oct 24, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htcfreek htcfreek htcfreek requested changes

@jaimecbernardo jaimecbernardo jaimecbernardo approved these changes

@crutkas crutkas crutkas approved these changes

@DHowett DHowett Awaiting requested review from DHowett DHowett is a code owner

@ethanfangg ethanfangg Awaiting requested review from ethanfangg ethanfangg is a code owner

Assignees
No one assigned
Labels
In for .86 Needs-Review This Pull Request awaits the review of a maintainer.
Projects
5. Needs Attention
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stefansjfw @htcfreek @jaimecbernardo @crutkas