Remove internal EventPipe API and redundant tests - #48053

Merged
sywhang merged 3 commits into
dotnet:masterfrom
sywhang:dev/suwhang/remove-eventpipe-root-type
Feb 10, 2021
Merged

Remove internal EventPipe API and redundant tests #48053
sywhang merged 3 commits into
dotnet:masterfrom
sywhang:dev/suwhang/remove-eventpipe-root-type

Conversation

@sywhang

@sywhangsywhang commented Feb 9, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

This removes the internal EventPipe type which was being used to turn on EventPipe session by an internal profiler team via reflection. They no longer depend on this as they've moved onto using the diagnostics client library.

Also this removes a few redundant tests that are using this internal API via reflection to enable tracing tests. I went and checked all of these tests and verified that they are either:

  • Testing the same scenario as the tests in tracing/eventpipe are testing
  • Testing the same scenario as the BCL EventSource tests are testing
  • Testing the internal reflection scenarios

The only test that is not already being tested by another tracing test is JittingStarted test which checks for whether a JittingStarted event is correctly received. I will file a separate issue to track adding this test under tracing/eventpipe.

cc @LakshanF After this PR, you should be able to safely remove the System.Diagnostics.Tracing.EventPipe type from rooted types for the ILTrim.

@sywhang
sywhang requested a review from josalemFebruary 9, 2021 13:04
@josalem

josalem commented Feb 9, 2021

Copy link
Copy Markdown
Contributor

CC @lateralusX

Glad to see these are no longer needed!

LakshanF
LakshanF approved these changes Feb 9, 2021
View reviewed changes

@LakshanFLakshanF left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Trimming HelloWorld app works with this change.

josalem
josalem approved these changes Feb 9, 2021
View reviewed changes

@josalemjosalem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The libraries test failure doesn't seem related to this patch. LGTM 🚀

Comment threadsrc/tests/tracing/tracevalidation/inducedgc/InducedGC.cs
{
public static class TraceValidationInducedGC
{
private static int InducedGCIterations = 10;

@josalemjosalemFeb 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure we have an equivalent test for the moder infrastructure. We may want to add one, but that's not part of this PR.

@sywhang
sywhang merged commit 998084d into dotnet:masterFeb 10, 2021
@sywhang
sywhang deleted the dev/suwhang/remove-eventpipe-root-type branch February 10, 2021 02:41
@LakshanFLakshanF mentioned this pull request Feb 10, 2021
Merged
@sywhangsywhang mentioned this pull request Feb 13, 2021
Merged
@sywhangsywhang mentioned this pull request Feb 27, 2021
Closed
@ghostghost locked as resolved and limited conversation to collaborators Mar 12, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@LakshanFLakshanFLakshanF approved these changes
@josalemjosalemjosalem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Tracing-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sywhang@josalem@LakshanF
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Remove internal EventPipe API and redundant tests  - #48053
Merged
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Feb 10, 2021
Merged
Remove internal EventPipe API and redundant tests #48053
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Conversation
@sywhang
@sywhangsywhang
commented
Feb 9, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
This removes the internal EventPipe type which was being used to turn on EventPipe session by an internal profiler team via reflection. They no longer depend on this as they've moved onto using the diagnostics client library.
Also this removes a few redundant tests that are using this internal API via reflection to enable tracing tests. I went and checked all of these tests and verified that they are either:
  • Testing the same scenario as the tests in tracing/eventpipe are testing
  • Testing the same scenario as the BCL EventSource tests are testing
  • Testing the internal reflection scenarios
The only test that is not already being tested by another tracing test is JittingStarted test which checks for whether a JittingStarted event is correctly received. I will file a separate issue to track adding this test under tracing/eventpipe.
cc @LakshanF After this PR, you should be able to safely remove the System.Diagnostics.Tracing.EventPipe type from rooted types for the ILTrim.
@sywhang
sywhang
requested a review
from josalemFebruary 9, 2021 13:04
@josalem
josalem
commented
Feb 9, 2021

Copy link


Copy Markdown

Contributor
CC @lateralusX
Glad to see these are no longer needed!
LakshanF
LakshanF
approved these changes
Feb 9, 2021
View reviewed changes
@LakshanFLakshanF
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Trimming HelloWorld app works with this change.
josalem
josalem
approved these changes
Feb 9, 2021
View reviewed changes
@josalemjosalem
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The libraries test failure doesn't seem related to this patch. LGTM 🚀
Comment threadsrc/tests/tracing/tracevalidation/inducedgc/InducedGC.cs
{
 public static class TraceValidationInducedGC
 {
 private static int InducedGCIterations = 10;
@josalemjosalemFeb 9, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm not sure we have an equivalent test for the moder infrastructure. We may want to add one, but that's not part of this PR.
@sywhang
sywhang
merged commit 998084d
into
dotnet:masterFeb 10, 2021
@sywhang
sywhang
deleted the

dev/suwhang/remove-eventpipe-root-type

branch
February 10, 2021 02:41
@LakshanFLakshanF
mentioned this pull request
Feb 10, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 13, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 27, 2021
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 12, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@LakshanFLakshanFLakshanF approved these changes
@josalemjosalemjosalem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Tracing-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sywhang@josalem@LakshanF

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove internal EventPipe API and redundant tests  - #48053
Merged
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Feb 10, 2021
Merged
Remove internal EventPipe API and redundant tests #48053
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Conversation
@sywhang
@sywhangsywhang
commented
Feb 9, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
This removes the internal EventPipe type which was being used to turn on EventPipe session by an internal profiler team via reflection. They no longer depend on this as they've moved onto using the diagnostics client library.
Also this removes a few redundant tests that are using this internal API via reflection to enable tracing tests. I went and checked all of these tests and verified that they are either:
  • Testing the same scenario as the tests in tracing/eventpipe are testing
  • Testing the same scenario as the BCL EventSource tests are testing
  • Testing the internal reflection scenarios
The only test that is not already being tested by another tracing test is JittingStarted test which checks for whether a JittingStarted event is correctly received. I will file a separate issue to track adding this test under tracing/eventpipe.
cc @LakshanF After this PR, you should be able to safely remove the System.Diagnostics.Tracing.EventPipe type from rooted types for the ILTrim.
@sywhang
sywhang
requested a review
from josalemFebruary 9, 2021 13:04
@josalem
josalem
commented
Feb 9, 2021

Copy link


Copy Markdown

Contributor
CC @lateralusX
Glad to see these are no longer needed!
LakshanF
LakshanF
approved these changes
Feb 9, 2021
View reviewed changes
@LakshanFLakshanF
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Trimming HelloWorld app works with this change.
josalem
josalem
approved these changes
Feb 9, 2021
View reviewed changes
@josalemjosalem
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The libraries test failure doesn't seem related to this patch. LGTM 🚀
Comment threadsrc/tests/tracing/tracevalidation/inducedgc/InducedGC.cs
{
 public static class TraceValidationInducedGC
 {
 private static int InducedGCIterations = 10;
@josalemjosalemFeb 9, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm not sure we have an equivalent test for the moder infrastructure. We may want to add one, but that's not part of this PR.
@sywhang
sywhang
merged commit 998084d
into
dotnet:masterFeb 10, 2021
@sywhang
sywhang
deleted the

dev/suwhang/remove-eventpipe-root-type

branch
February 10, 2021 02:41
@LakshanFLakshanF
mentioned this pull request
Feb 10, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 13, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 27, 2021
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 12, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@LakshanFLakshanFLakshanF approved these changes
@josalemjosalemjosalem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Tracing-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sywhang@josalem@LakshanF

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove internal EventPipe API and redundant tests  - #48053
Merged
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Feb 10, 2021
Merged
Remove internal EventPipe API and redundant tests #48053
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Conversation
@sywhang
@sywhangsywhang
commented
Feb 9, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
This removes the internal EventPipe type which was being used to turn on EventPipe session by an internal profiler team via reflection. They no longer depend on this as they've moved onto using the diagnostics client library.
Also this removes a few redundant tests that are using this internal API via reflection to enable tracing tests. I went and checked all of these tests and verified that they are either:
  • Testing the same scenario as the tests in tracing/eventpipe are testing
  • Testing the same scenario as the BCL EventSource tests are testing
  • Testing the internal reflection scenarios
The only test that is not already being tested by another tracing test is JittingStarted test which checks for whether a JittingStarted event is correctly received. I will file a separate issue to track adding this test under tracing/eventpipe.
cc @LakshanF After this PR, you should be able to safely remove the System.Diagnostics.Tracing.EventPipe type from rooted types for the ILTrim.
@sywhang
sywhang
requested a review
from josalemFebruary 9, 2021 13:04
@josalem
josalem
commented
Feb 9, 2021

Copy link


Copy Markdown

Contributor
CC @lateralusX
Glad to see these are no longer needed!
LakshanF
LakshanF
approved these changes
Feb 9, 2021
View reviewed changes
@LakshanFLakshanF
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Trimming HelloWorld app works with this change.
josalem
josalem
approved these changes
Feb 9, 2021
View reviewed changes
@josalemjosalem
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The libraries test failure doesn't seem related to this patch. LGTM 🚀
Comment threadsrc/tests/tracing/tracevalidation/inducedgc/InducedGC.cs
{
 public static class TraceValidationInducedGC
 {
 private static int InducedGCIterations = 10;
@josalemjosalemFeb 9, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm not sure we have an equivalent test for the moder infrastructure. We may want to add one, but that's not part of this PR.
@sywhang
sywhang
merged commit 998084d
into
dotnet:masterFeb 10, 2021
@sywhang
sywhang
deleted the

dev/suwhang/remove-eventpipe-root-type

branch
February 10, 2021 02:41
@LakshanFLakshanF
mentioned this pull request
Feb 10, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 13, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 27, 2021
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 12, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@LakshanFLakshanFLakshanF approved these changes
@josalemjosalemjosalem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Tracing-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sywhang@josalem@LakshanF

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Remove internal EventPipe API and redundant tests  - #48053
Merged
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Feb 10, 2021
Merged
Remove internal EventPipe API and redundant tests #48053
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Conversation
@sywhang
@sywhangsywhang
commented
Feb 9, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
This removes the internal EventPipe type which was being used to turn on EventPipe session by an internal profiler team via reflection. They no longer depend on this as they've moved onto using the diagnostics client library.
Also this removes a few redundant tests that are using this internal API via reflection to enable tracing tests. I went and checked all of these tests and verified that they are either:
  • Testing the same scenario as the tests in tracing/eventpipe are testing
  • Testing the same scenario as the BCL EventSource tests are testing
  • Testing the internal reflection scenarios
The only test that is not already being tested by another tracing test is JittingStarted test which checks for whether a JittingStarted event is correctly received. I will file a separate issue to track adding this test under tracing/eventpipe.
cc @LakshanF After this PR, you should be able to safely remove the System.Diagnostics.Tracing.EventPipe type from rooted types for the ILTrim.
@sywhang
sywhang
requested a review
from josalemFebruary 9, 2021 13:04
@josalem
josalem
commented
Feb 9, 2021

Copy link


Copy Markdown

Contributor
CC @lateralusX
Glad to see these are no longer needed!
LakshanF
LakshanF
approved these changes
Feb 9, 2021
View reviewed changes
@LakshanFLakshanF
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Trimming HelloWorld app works with this change.
josalem
josalem
approved these changes
Feb 9, 2021
View reviewed changes
@josalemjosalem
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The libraries test failure doesn't seem related to this patch. LGTM 🚀
Comment threadsrc/tests/tracing/tracevalidation/inducedgc/InducedGC.cs
{
 public static class TraceValidationInducedGC
 {
 private static int InducedGCIterations = 10;
@josalemjosalemFeb 9, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm not sure we have an equivalent test for the moder infrastructure. We may want to add one, but that's not part of this PR.
@sywhang
sywhang
merged commit 998084d
into
dotnet:masterFeb 10, 2021
@sywhang
sywhang
deleted the

dev/suwhang/remove-eventpipe-root-type

branch
February 10, 2021 02:41
@LakshanFLakshanF
mentioned this pull request
Feb 10, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 13, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 27, 2021
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 12, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@LakshanFLakshanFLakshanF approved these changes
@josalemjosalemjosalem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Tracing-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sywhang@josalem@LakshanF

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove internal EventPipe API and redundant tests  - #48053
Merged
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Feb 10, 2021
Merged
Remove internal EventPipe API and redundant tests #48053
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Conversation
@sywhang
@sywhangsywhang
commented
Feb 9, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
This removes the internal EventPipe type which was being used to turn on EventPipe session by an internal profiler team via reflection. They no longer depend on this as they've moved onto using the diagnostics client library.
Also this removes a few redundant tests that are using this internal API via reflection to enable tracing tests. I went and checked all of these tests and verified that they are either:
  • Testing the same scenario as the tests in tracing/eventpipe are testing
  • Testing the same scenario as the BCL EventSource tests are testing
  • Testing the internal reflection scenarios
The only test that is not already being tested by another tracing test is JittingStarted test which checks for whether a JittingStarted event is correctly received. I will file a separate issue to track adding this test under tracing/eventpipe.
cc @LakshanF After this PR, you should be able to safely remove the System.Diagnostics.Tracing.EventPipe type from rooted types for the ILTrim.
@sywhang
sywhang
requested a review
from josalemFebruary 9, 2021 13:04
@josalem
josalem
commented
Feb 9, 2021

Copy link


Copy Markdown

Contributor
CC @lateralusX
Glad to see these are no longer needed!
LakshanF
LakshanF
approved these changes
Feb 9, 2021
View reviewed changes
@LakshanFLakshanF
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Trimming HelloWorld app works with this change.
josalem
josalem
approved these changes
Feb 9, 2021
View reviewed changes
@josalemjosalem
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The libraries test failure doesn't seem related to this patch. LGTM 🚀
Comment threadsrc/tests/tracing/tracevalidation/inducedgc/InducedGC.cs
{
 public static class TraceValidationInducedGC
 {
 private static int InducedGCIterations = 10;
@josalemjosalemFeb 9, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm not sure we have an equivalent test for the moder infrastructure. We may want to add one, but that's not part of this PR.
@sywhang
sywhang
merged commit 998084d
into
dotnet:masterFeb 10, 2021
@sywhang
sywhang
deleted the

dev/suwhang/remove-eventpipe-root-type

branch
February 10, 2021 02:41
@LakshanFLakshanF
mentioned this pull request
Feb 10, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 13, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 27, 2021
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 12, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@LakshanFLakshanFLakshanF approved these changes
@josalemjosalemjosalem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Tracing-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sywhang@josalem@LakshanF

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove internal EventPipe API and redundant tests  - #48053
Merged
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Feb 10, 2021
Merged
Remove internal EventPipe API and redundant tests #48053
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Conversation
@sywhang
@sywhangsywhang
commented
Feb 9, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
This removes the internal EventPipe type which was being used to turn on EventPipe session by an internal profiler team via reflection. They no longer depend on this as they've moved onto using the diagnostics client library.
Also this removes a few redundant tests that are using this internal API via reflection to enable tracing tests. I went and checked all of these tests and verified that they are either:
  • Testing the same scenario as the tests in tracing/eventpipe are testing
  • Testing the same scenario as the BCL EventSource tests are testing
  • Testing the internal reflection scenarios
The only test that is not already being tested by another tracing test is JittingStarted test which checks for whether a JittingStarted event is correctly received. I will file a separate issue to track adding this test under tracing/eventpipe.
cc @LakshanF After this PR, you should be able to safely remove the System.Diagnostics.Tracing.EventPipe type from rooted types for the ILTrim.
@sywhang
sywhang
requested a review
from josalemFebruary 9, 2021 13:04
@josalem
josalem
commented
Feb 9, 2021

Copy link


Copy Markdown

Contributor
CC @lateralusX
Glad to see these are no longer needed!
LakshanF
LakshanF
approved these changes
Feb 9, 2021
View reviewed changes
@LakshanFLakshanF
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Trimming HelloWorld app works with this change.
josalem
josalem
approved these changes
Feb 9, 2021
View reviewed changes
@josalemjosalem
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The libraries test failure doesn't seem related to this patch. LGTM 🚀
Comment threadsrc/tests/tracing/tracevalidation/inducedgc/InducedGC.cs
{
 public static class TraceValidationInducedGC
 {
 private static int InducedGCIterations = 10;
@josalemjosalemFeb 9, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm not sure we have an equivalent test for the moder infrastructure. We may want to add one, but that's not part of this PR.
@sywhang
sywhang
merged commit 998084d
into
dotnet:masterFeb 10, 2021
@sywhang
sywhang
deleted the

dev/suwhang/remove-eventpipe-root-type

branch
February 10, 2021 02:41
@LakshanFLakshanF
mentioned this pull request
Feb 10, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 13, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 27, 2021
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 12, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@LakshanFLakshanFLakshanF approved these changes
@josalemjosalemjosalem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Tracing-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sywhang@josalem@LakshanF

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Remove internal EventPipe API and redundant tests  - #48053
Merged
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Feb 10, 2021
Merged
Remove internal EventPipe API and redundant tests #48053
sywhang merged 3 commits into
dotnet:masterfrom 
sywhang:dev/suwhang/remove-eventpipe-root-type
Conversation
@sywhang
@sywhangsywhang
commented
Feb 9, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
This removes the internal EventPipe type which was being used to turn on EventPipe session by an internal profiler team via reflection. They no longer depend on this as they've moved onto using the diagnostics client library.
Also this removes a few redundant tests that are using this internal API via reflection to enable tracing tests. I went and checked all of these tests and verified that they are either:
  • Testing the same scenario as the tests in tracing/eventpipe are testing
  • Testing the same scenario as the BCL EventSource tests are testing
  • Testing the internal reflection scenarios
The only test that is not already being tested by another tracing test is JittingStarted test which checks for whether a JittingStarted event is correctly received. I will file a separate issue to track adding this test under tracing/eventpipe.
cc @LakshanF After this PR, you should be able to safely remove the System.Diagnostics.Tracing.EventPipe type from rooted types for the ILTrim.
@sywhang
sywhang
requested a review
from josalemFebruary 9, 2021 13:04
@josalem
josalem
commented
Feb 9, 2021

Copy link


Copy Markdown

Contributor
CC @lateralusX
Glad to see these are no longer needed!
LakshanF
LakshanF
approved these changes
Feb 9, 2021
View reviewed changes
@LakshanFLakshanF
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Trimming HelloWorld app works with this change.
josalem
josalem
approved these changes
Feb 9, 2021
View reviewed changes
@josalemjosalem
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The libraries test failure doesn't seem related to this patch. LGTM 🚀
Comment threadsrc/tests/tracing/tracevalidation/inducedgc/InducedGC.cs
{
 public static class TraceValidationInducedGC
 {
 private static int InducedGCIterations = 10;
@josalemjosalemFeb 9, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm not sure we have an equivalent test for the moder infrastructure. We may want to add one, but that's not part of this PR.
@sywhang
sywhang
merged commit 998084d
into
dotnet:masterFeb 10, 2021
@sywhang
sywhang
deleted the

dev/suwhang/remove-eventpipe-root-type

branch
February 10, 2021 02:41
@LakshanFLakshanF
mentioned this pull request
Feb 10, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 13, 2021
 Merged

@sywhangsywhang
mentioned this pull request
Feb 27, 2021
 Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 12, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@LakshanFLakshanFLakshanF approved these changes
@josalemjosalemjosalem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-Tracing-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sywhang@josalem@LakshanF