Move diagnostic-only events to separate providers #10093
Assignees
Labels
Area-Performance
Performance-related issue
Issue-Bug
It either shouldn't be doing this or needs an investigation.
Product-Conhost
For issues in the Console codebase
Product-Terminal
The new Windows Terminal.
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Comments
miniksa
added
Product-Conhost
ghost
added
the
Needs-Triage
zadjii-msft
removed
the
Needs-Triage
8 tasks
ghost
pushed a commit
that referenced
this issue
May 14, 2021
Set keyword flags on all events so those sharing a provider with telemetry do not fire unless tracing is enabled ## PR Checklist * [x] Closes #10093 * [x] I work here * [x] Tests passed * [x] Documentation added in `til.h` about how keywords work and at the only other site of keywords we define in the Host project tracing files. ## Detailed Description of the Pull Request / Additional comments I initially thought that we would need to split providers here to accomplish this... but @DHowett helped me realize that might be a lot of additional metadata and bloat binary size. So with help from a friend from fundamentals, I realized that we could use Keywords to differentiate here. We can no longer define 0 keywords as that represents an any/all scenario. Every `TraceLoggingWrite` event now needs a keyword. When our events have a keyword, they're not included in any trace. Additionally, when we have an explicit keyword to check that is different from the ones used for the telemetry pipeline, we can ensure that we only do "hard work" to generate debug trace data when an "ALL" type listener like TraceView or Windows Performance Recorder with our profiles is listening to these providers for ALL keyworded events. ## Validation Steps Performed - [x] - Built with full release build config to confirm performance is worse than dev builds BECAUSE of the telemetry event collector camping our provider and triggering full trace event generation on shared providers. - [x] - Built with full release build config to enable statistics collection and validated trace event collection is excluded and trace event short-circuits work with this change. - [x] - Checked that TraceView still sees both telemetry and tracing events - [x] - Checked that WPR with our .wprp profile sees both telemetry and tracing events
ghost
added
the
Resolution-Fix-Committed
DHowett
pushed a commit
that referenced
this issue
May 17, 2021
Set keyword flags on all events so those sharing a provider with telemetry do not fire unless tracing is enabled ## PR Checklist * [x] Closes #10093 * [x] I work here * [x] Tests passed * [x] Documentation added in `til.h` about how keywords work and at the only other site of keywords we define in the Host project tracing files. ## Detailed Description of the Pull Request / Additional comments I initially thought that we would need to split providers here to accomplish this... but @DHowett helped me realize that might be a lot of additional metadata and bloat binary size. So with help from a friend from fundamentals, I realized that we could use Keywords to differentiate here. We can no longer define 0 keywords as that represents an any/all scenario. Every `TraceLoggingWrite` event now needs a keyword. When our events have a keyword, they're not included in any trace. Additionally, when we have an explicit keyword to check that is different from the ones used for the telemetry pipeline, we can ensure that we only do "hard work" to generate debug trace data when an "ALL" type listener like TraceView or Windows Performance Recorder with our profiles is listening to these providers for ALL keyworded events. ## Validation Steps Performed - [x] - Built with full release build config to confirm performance is worse than dev builds BECAUSE of the telemetry event collector camping our provider and triggering full trace event generation on shared providers. - [x] - Built with full release build config to enable statistics collection and validated trace event collection is excluded and trace event short-circuits work with this change. - [x] - Checked that TraceView still sees both telemetry and tracing events - [x] - Checked that WPR with our .wprp profile sees both telemetry and tracing events (cherry picked from commit 66fdc64) # Conflicts: # src/cascadia/Remoting/Monarch.cpp # src/cascadia/Remoting/Peasant.cpp # src/cascadia/WindowsTerminal/IslandWindow.cpp
DHowett
pushed a commit
that referenced
this issue
May 17, 2021
Set keyword flags on all events so those sharing a provider with telemetry do not fire unless tracing is enabled ## PR Checklist * [x] Closes #10093 * [x] I work here * [x] Tests passed * [x] Documentation added in `til.h` about how keywords work and at the only other site of keywords we define in the Host project tracing files. ## Detailed Description of the Pull Request / Additional comments I initially thought that we would need to split providers here to accomplish this... but @DHowett helped me realize that might be a lot of additional metadata and bloat binary size. So with help from a friend from fundamentals, I realized that we could use Keywords to differentiate here. We can no longer define 0 keywords as that represents an any/all scenario. Every `TraceLoggingWrite` event now needs a keyword. When our events have a keyword, they're not included in any trace. Additionally, when we have an explicit keyword to check that is different from the ones used for the telemetry pipeline, we can ensure that we only do "hard work" to generate debug trace data when an "ALL" type listener like TraceView or Windows Performance Recorder with our profiles is listening to these providers for ALL keyworded events. ## Validation Steps Performed - [x] - Built with full release build config to confirm performance is worse than dev builds BECAUSE of the telemetry event collector camping our provider and triggering full trace event generation on shared providers. - [x] - Built with full release build config to enable statistics collection and validated trace event collection is excluded and trace event short-circuits work with this change. - [x] - Checked that TraceView still sees both telemetry and tracing events - [x] - Checked that WPR with our .wprp profile sees both telemetry and tracing events (cherry picked from commit 66fdc64) # Conflicts: # src/Terminal.wprp # src/cascadia/Remoting/Monarch.cpp # src/cascadia/Remoting/Peasant.cpp # src/cascadia/Remoting/WindowManager.cpp # src/cascadia/WindowsTerminal/IslandWindow.cpp # src/host/exe/exemain.cpp
|
🎉This issue was addressed in #10098, which has now been successfully released as Handy links: |
|
🎉This issue was addressed in #10098, which has now been successfully released as Handy links: |
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Turns out when we send diagnostic events on providers intended for other analysis... they can get listened to more than we expect and slow down the entire execution of the program.
This represents scrubbing through our expensive diagnostic-only events and moving them to their own providers (as correcting troubleshooting and data providers on the back end is more difficult, so we will leave those ones in place on the providers.)
The text was updated successfully, but these errors were encountered: