Add COMPlus_DiagnosticsServerTransportPath - #1600

Merged
josalem merged 4 commits into
dotnet:masterfrom
josalem:dev/josalem/diagnostics-socket-name
Jan 24, 2020
Merged

Add COMPlus_DiagnosticsServerTransportPath#1600
josalem merged 4 commits into
dotnet:masterfrom
josalem:dev/josalem/diagnostics-socket-name

Conversation

@josalem

@josalemjosalem commented Jan 10, 2020

Copy link
Copy Markdown
Contributor
  • allows users to specify a location for the Diagnostics Server

This change allows users to change the location of the Diagnostics Server's OS Transport (Named Pipe on Windows and Unix Domain Socket on Linux).

I'm doing some end-to-end testing with an updated version of the .NET Global Diagnostics Tools that allows connecting to a specific pipe. I have tested this on Windows successfully and will be checking on Linux soon (I'll update this PR with the results).

CC - @tommcdon, @mikem8361

@josalemjosalem added this to the 5.0 milestone Jan 10, 2020
@josalemjosalem self-assigned this Jan 10, 2020
noahfalk
noahfalk requested changes Jan 11, 2020
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm requesting a little refactoring, but I think you are on the right track

Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp Outdated

@noahfalknoahfalkJan 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should keep the COMPlus env var handling consolidated in clrconfigvalues.h and then read it with standard accessor (CLRConfig::GetConfigValue). You can pass in the alternate name as a string parameter and read it using code in the VM, for example in DiagnosticServer::Initialize() right next to where we read the EnableDiagnostics variable.

@josalemjosalemJan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I refactored it into DiagnosticServer::Initialize.

Comment threadsrc/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp Outdated

@noahfalknoahfalkJan 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same deal for the env var

Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp Outdated

@noahfalknoahfalkJan 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It isn't ideal that it will be an either/or choice between connecting via PID or connecting via a custom path. I think it is a reasonable place to start though and we can wait for feedback. The ideal (which would take a good bit more work) is to listen to both the custom and default pipes simultaneously rather forcing a choice between them.

@josalemjosalemJan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree. I'll look into what it would take to do this. I imagine we could just create both sockets and then use a select like construct to call accept on both.

@noahfalknoahfalkJan 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll look into what it would take to do this

Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.

@josalemjosalem mentioned this pull request Jan 13, 2020
Closed
sywhang
sywhang approved these changes Jan 14, 2020
View reviewed changes

@sywhangsywhang 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.

LGTM

noahfalk
noahfalk approved these changes Jan 14, 2020
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp Outdated

@noahfalknoahfalkJan 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll look into what it would take to do this

Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.

@josalem
josalemforce-pushed the dev/josalem/diagnostics-socket-name branch from cac3baa to e7dd773CompareJanuary 15, 2020 22:42
@josalem

josalem commented Jan 15, 2020

Copy link
Copy Markdown
ContributorAuthor

Rebasing onto master in an attempt to get tests running. For some reason Windows CoreCLR and Libraries tests weren't running. Or at least the test logs weren't getting generated, which caused the python reporter scripts to fail with no test logs.

@josalemjosalem mentioned this pull request Jan 16, 2020
Closed
@josalem
josalemforce-pushed the dev/josalem/diagnostics-socket-name branch from e7dd773 to 894b3d6CompareJanuary 16, 2020 01:20
@josalem

josalem commented Jan 16, 2020

Copy link
Copy Markdown
ContributorAuthor

Closing and reopening to use newest pipeline

@josalemjosalem closed this Jan 16, 2020
@josalemjosalem reopened this Jan 16, 2020
@josalem

josalem commented Jan 22, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime

@azure-pipelines

azure-pipelinesBot commented Jan 22, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@josalemjosalem mentioned this pull request Jan 22, 2020
Closed
@josalem

josalem commented Jan 22, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime

@azure-pipelines

azure-pipelinesBot commented Jan 22, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@josalemjosalem mentioned this pull request Jan 22, 2020
Closed
@josalem
josalemforce-pushed the dev/josalem/diagnostics-socket-name branch from b2f7a5b to 68bf134CompareJanuary 22, 2020 22:48
@josalemjosalem closed this Jan 22, 2020
@josalemjosalem reopened this Jan 22, 2020
@josalem

josalem commented Jan 23, 2020

Copy link
Copy Markdown
ContributorAuthor

I believe the remaining errors are #1089 since I kicked off the build around 3:30pm and 4pm PST is midnight UST. Will rerun with azp.

/azp run runtime

@safern

safern commented Jan 23, 2020

Copy link
Copy Markdown
Member

/azp run runtime

@azure-pipelines

azure-pipelinesBot commented Jan 23, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@josalem
josalem merged commit fd5f7a7 into dotnet:masterJan 24, 2020
@josalem
josalem deleted the dev/josalem/diagnostics-socket-name branch January 24, 2020 00:27
@josalemjosalem mentioned this pull request Feb 8, 2020
Closed
@shirhattishirhatti mentioned this pull request Jun 4, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@noahfalknoahfalknoahfalk approved these changes
+2 more reviewers
@mikem8361mikem8361mikem8361 approved these changes
@sywhangsywhangsywhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclrEventPipe

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem@safern@noahfalk@mikem8361@sywhang@Dotnet-GitSync-Bot
, '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
Add COMPlus_DiagnosticsServerTransportPath - #1600
Merged
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Jan 24, 2020
Merged
Add COMPlus_DiagnosticsServerTransportPath#1600
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Conversation
@josalem
@josalemjosalem
commented
Jan 10, 2020

Copy link


Copy Markdown

Contributor
  • allows users to specify a location for the Diagnostics Server
This change allows users to change the location of the Diagnostics Server's OS Transport (Named Pipe on Windows and Unix Domain Socket on Linux).
I'm doing some end-to-end testing with an updated version of the .NET Global Diagnostics Tools that allows connecting to a specific pipe. I have tested this on Windows successfully and will be checking on Linux soon (I'll update this PR with the results).
CC - @tommcdon, @mikem8361
@josalemjosalem
added this to the 5.0 milestone
Jan 10, 2020
@josalemjosalem
self-assigned this
Jan 10, 2020
noahfalk
noahfalk
requested changes
Jan 11, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'm requesting a little refactoring, but I think you are on the right track
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should keep the COMPlus env var handling consolidated in clrconfigvalues.h and then read it with standard accessor (CLRConfig::GetConfigValue). You can pass in the alternate name as a string parameter and read it using code in the VM, for example in DiagnosticServer::Initialize() right next to where we read the EnableDiagnostics variable.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I refactored it into DiagnosticServer::Initialize.
Comment threadsrc/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Same deal for the env var
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It isn't ideal that it will be an either/or choice between connecting via PID or connecting via a custom path. I think it is a reasonable place to start though and we can wait for feedback. The ideal (which would take a good bit more work) is to listen to both the custom and default pipes simultaneously rather forcing a choice between them.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I agree. I'll look into what it would take to do this. I imagine we could just create both sockets and then use a select like construct to call accept on both.
@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalemjosalem
mentioned this pull request
Jan 13, 2020

Closed

sywhang
sywhang
approved these changes
Jan 14, 2020
View reviewed changes
@sywhangsywhang
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.

LGTM
noahfalk
noahfalk
approved these changes
Jan 14, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM 👍
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
cac3baa to
e7dd773CompareJanuary 15, 2020 22:42
@josalem
josalem
commented
Jan 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebasing onto master in an attempt to get tests running. For some reason Windows CoreCLR and Libraries tests weren't running. Or at least the test logs weren't getting generated, which caused the python reporter scripts to fail with no test logs.
@josalemjosalem
mentioned this pull request
Jan 16, 2020

Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
e7dd773 to
894b3d6CompareJanuary 16, 2020 01:20
@josalem
josalem
commented
Jan 16, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing and reopening to use newest pipeline
@josalemjosalem
closed this
Jan 16, 2020
@josalemjosalem
reopened this
Jan 16, 2020
@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020

Closed

@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020
 Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
b2f7a5b to
68bf134CompareJanuary 22, 2020 22:48
@josalemjosalem
closed this
Jan 22, 2020
@josalemjosalem
reopened this
Jan 22, 2020
@josalem
josalem
commented
Jan 23, 2020

Copy link


Copy Markdown

ContributorAuthor
I believe the remaining errors are #1089 since I kicked off the build around 3:30pm and 4pm PST is midnight UST. Will rerun with azp.
/azp run runtime
@safern
safern
commented
Jan 23, 2020

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 23, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalem
josalem
merged commit fd5f7a7
into
dotnet:masterJan 24, 2020
@josalem
josalem
deleted the

dev/josalem/diagnostics-socket-name

branch
January 24, 2020 00:27
@josalemjosalem
mentioned this pull request
Feb 8, 2020

Closed

@shirhattishirhatti
mentioned this pull request
Jun 4, 2020

Closed

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


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+2 more reviewers

@mikem8361mikem8361mikem8361 approved these changes
@sywhangsywhangsywhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclrEventPipe

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem@safern@noahfalk@mikem8361@sywhang@Dotnet-GitSync-Bot

, '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
Add COMPlus_DiagnosticsServerTransportPath - #1600
Merged
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Jan 24, 2020
Merged
Add COMPlus_DiagnosticsServerTransportPath#1600
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Conversation
@josalem
@josalemjosalem
commented
Jan 10, 2020

Copy link


Copy Markdown

Contributor
  • allows users to specify a location for the Diagnostics Server
This change allows users to change the location of the Diagnostics Server's OS Transport (Named Pipe on Windows and Unix Domain Socket on Linux).
I'm doing some end-to-end testing with an updated version of the .NET Global Diagnostics Tools that allows connecting to a specific pipe. I have tested this on Windows successfully and will be checking on Linux soon (I'll update this PR with the results).
CC - @tommcdon, @mikem8361
@josalemjosalem
added this to the 5.0 milestone
Jan 10, 2020
@josalemjosalem
self-assigned this
Jan 10, 2020
noahfalk
noahfalk
requested changes
Jan 11, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'm requesting a little refactoring, but I think you are on the right track
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should keep the COMPlus env var handling consolidated in clrconfigvalues.h and then read it with standard accessor (CLRConfig::GetConfigValue). You can pass in the alternate name as a string parameter and read it using code in the VM, for example in DiagnosticServer::Initialize() right next to where we read the EnableDiagnostics variable.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I refactored it into DiagnosticServer::Initialize.
Comment threadsrc/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Same deal for the env var
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It isn't ideal that it will be an either/or choice between connecting via PID or connecting via a custom path. I think it is a reasonable place to start though and we can wait for feedback. The ideal (which would take a good bit more work) is to listen to both the custom and default pipes simultaneously rather forcing a choice between them.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I agree. I'll look into what it would take to do this. I imagine we could just create both sockets and then use a select like construct to call accept on both.
@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalemjosalem
mentioned this pull request
Jan 13, 2020

Closed

sywhang
sywhang
approved these changes
Jan 14, 2020
View reviewed changes
@sywhangsywhang
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.

LGTM
noahfalk
noahfalk
approved these changes
Jan 14, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM 👍
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
cac3baa to
e7dd773CompareJanuary 15, 2020 22:42
@josalem
josalem
commented
Jan 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebasing onto master in an attempt to get tests running. For some reason Windows CoreCLR and Libraries tests weren't running. Or at least the test logs weren't getting generated, which caused the python reporter scripts to fail with no test logs.
@josalemjosalem
mentioned this pull request
Jan 16, 2020

Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
e7dd773 to
894b3d6CompareJanuary 16, 2020 01:20
@josalem
josalem
commented
Jan 16, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing and reopening to use newest pipeline
@josalemjosalem
closed this
Jan 16, 2020
@josalemjosalem
reopened this
Jan 16, 2020
@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020

Closed

@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020
 Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
b2f7a5b to
68bf134CompareJanuary 22, 2020 22:48
@josalemjosalem
closed this
Jan 22, 2020
@josalemjosalem
reopened this
Jan 22, 2020
@josalem
josalem
commented
Jan 23, 2020

Copy link


Copy Markdown

ContributorAuthor
I believe the remaining errors are #1089 since I kicked off the build around 3:30pm and 4pm PST is midnight UST. Will rerun with azp.
/azp run runtime
@safern
safern
commented
Jan 23, 2020

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 23, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalem
josalem
merged commit fd5f7a7
into
dotnet:masterJan 24, 2020
@josalem
josalem
deleted the

dev/josalem/diagnostics-socket-name

branch
January 24, 2020 00:27
@josalemjosalem
mentioned this pull request
Feb 8, 2020

Closed

@shirhattishirhatti
mentioned this pull request
Jun 4, 2020

Closed

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


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+2 more reviewers

@mikem8361mikem8361mikem8361 approved these changes
@sywhangsywhangsywhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclrEventPipe

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem@safern@noahfalk@mikem8361@sywhang@Dotnet-GitSync-Bot

, '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
Add COMPlus_DiagnosticsServerTransportPath - #1600
Merged
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Jan 24, 2020
Merged
Add COMPlus_DiagnosticsServerTransportPath#1600
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Conversation
@josalem
@josalemjosalem
commented
Jan 10, 2020

Copy link


Copy Markdown

Contributor
  • allows users to specify a location for the Diagnostics Server
This change allows users to change the location of the Diagnostics Server's OS Transport (Named Pipe on Windows and Unix Domain Socket on Linux).
I'm doing some end-to-end testing with an updated version of the .NET Global Diagnostics Tools that allows connecting to a specific pipe. I have tested this on Windows successfully and will be checking on Linux soon (I'll update this PR with the results).
CC - @tommcdon, @mikem8361
@josalemjosalem
added this to the 5.0 milestone
Jan 10, 2020
@josalemjosalem
self-assigned this
Jan 10, 2020
noahfalk
noahfalk
requested changes
Jan 11, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'm requesting a little refactoring, but I think you are on the right track
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should keep the COMPlus env var handling consolidated in clrconfigvalues.h and then read it with standard accessor (CLRConfig::GetConfigValue). You can pass in the alternate name as a string parameter and read it using code in the VM, for example in DiagnosticServer::Initialize() right next to where we read the EnableDiagnostics variable.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I refactored it into DiagnosticServer::Initialize.
Comment threadsrc/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Same deal for the env var
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It isn't ideal that it will be an either/or choice between connecting via PID or connecting via a custom path. I think it is a reasonable place to start though and we can wait for feedback. The ideal (which would take a good bit more work) is to listen to both the custom and default pipes simultaneously rather forcing a choice between them.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I agree. I'll look into what it would take to do this. I imagine we could just create both sockets and then use a select like construct to call accept on both.
@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalemjosalem
mentioned this pull request
Jan 13, 2020

Closed

sywhang
sywhang
approved these changes
Jan 14, 2020
View reviewed changes
@sywhangsywhang
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.

LGTM
noahfalk
noahfalk
approved these changes
Jan 14, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM 👍
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
cac3baa to
e7dd773CompareJanuary 15, 2020 22:42
@josalem
josalem
commented
Jan 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebasing onto master in an attempt to get tests running. For some reason Windows CoreCLR and Libraries tests weren't running. Or at least the test logs weren't getting generated, which caused the python reporter scripts to fail with no test logs.
@josalemjosalem
mentioned this pull request
Jan 16, 2020

Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
e7dd773 to
894b3d6CompareJanuary 16, 2020 01:20
@josalem
josalem
commented
Jan 16, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing and reopening to use newest pipeline
@josalemjosalem
closed this
Jan 16, 2020
@josalemjosalem
reopened this
Jan 16, 2020
@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020

Closed

@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020
 Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
b2f7a5b to
68bf134CompareJanuary 22, 2020 22:48
@josalemjosalem
closed this
Jan 22, 2020
@josalemjosalem
reopened this
Jan 22, 2020
@josalem
josalem
commented
Jan 23, 2020

Copy link


Copy Markdown

ContributorAuthor
I believe the remaining errors are #1089 since I kicked off the build around 3:30pm and 4pm PST is midnight UST. Will rerun with azp.
/azp run runtime
@safern
safern
commented
Jan 23, 2020

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 23, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalem
josalem
merged commit fd5f7a7
into
dotnet:masterJan 24, 2020
@josalem
josalem
deleted the

dev/josalem/diagnostics-socket-name

branch
January 24, 2020 00:27
@josalemjosalem
mentioned this pull request
Feb 8, 2020

Closed

@shirhattishirhatti
mentioned this pull request
Jun 4, 2020

Closed

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


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+2 more reviewers

@mikem8361mikem8361mikem8361 approved these changes
@sywhangsywhangsywhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclrEventPipe

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem@safern@noahfalk@mikem8361@sywhang@Dotnet-GitSync-Bot

, '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
Add COMPlus_DiagnosticsServerTransportPath - #1600
Merged
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Jan 24, 2020
Merged
Add COMPlus_DiagnosticsServerTransportPath#1600
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Conversation
@josalem
@josalemjosalem
commented
Jan 10, 2020

Copy link


Copy Markdown

Contributor
  • allows users to specify a location for the Diagnostics Server
This change allows users to change the location of the Diagnostics Server's OS Transport (Named Pipe on Windows and Unix Domain Socket on Linux).
I'm doing some end-to-end testing with an updated version of the .NET Global Diagnostics Tools that allows connecting to a specific pipe. I have tested this on Windows successfully and will be checking on Linux soon (I'll update this PR with the results).
CC - @tommcdon, @mikem8361
@josalemjosalem
added this to the 5.0 milestone
Jan 10, 2020
@josalemjosalem
self-assigned this
Jan 10, 2020
noahfalk
noahfalk
requested changes
Jan 11, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'm requesting a little refactoring, but I think you are on the right track
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should keep the COMPlus env var handling consolidated in clrconfigvalues.h and then read it with standard accessor (CLRConfig::GetConfigValue). You can pass in the alternate name as a string parameter and read it using code in the VM, for example in DiagnosticServer::Initialize() right next to where we read the EnableDiagnostics variable.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I refactored it into DiagnosticServer::Initialize.
Comment threadsrc/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Same deal for the env var
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It isn't ideal that it will be an either/or choice between connecting via PID or connecting via a custom path. I think it is a reasonable place to start though and we can wait for feedback. The ideal (which would take a good bit more work) is to listen to both the custom and default pipes simultaneously rather forcing a choice between them.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I agree. I'll look into what it would take to do this. I imagine we could just create both sockets and then use a select like construct to call accept on both.
@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalemjosalem
mentioned this pull request
Jan 13, 2020

Closed

sywhang
sywhang
approved these changes
Jan 14, 2020
View reviewed changes
@sywhangsywhang
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.

LGTM
noahfalk
noahfalk
approved these changes
Jan 14, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM 👍
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
cac3baa to
e7dd773CompareJanuary 15, 2020 22:42
@josalem
josalem
commented
Jan 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebasing onto master in an attempt to get tests running. For some reason Windows CoreCLR and Libraries tests weren't running. Or at least the test logs weren't getting generated, which caused the python reporter scripts to fail with no test logs.
@josalemjosalem
mentioned this pull request
Jan 16, 2020

Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
e7dd773 to
894b3d6CompareJanuary 16, 2020 01:20
@josalem
josalem
commented
Jan 16, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing and reopening to use newest pipeline
@josalemjosalem
closed this
Jan 16, 2020
@josalemjosalem
reopened this
Jan 16, 2020
@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020

Closed

@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020
 Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
b2f7a5b to
68bf134CompareJanuary 22, 2020 22:48
@josalemjosalem
closed this
Jan 22, 2020
@josalemjosalem
reopened this
Jan 22, 2020
@josalem
josalem
commented
Jan 23, 2020

Copy link


Copy Markdown

ContributorAuthor
I believe the remaining errors are #1089 since I kicked off the build around 3:30pm and 4pm PST is midnight UST. Will rerun with azp.
/azp run runtime
@safern
safern
commented
Jan 23, 2020

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 23, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalem
josalem
merged commit fd5f7a7
into
dotnet:masterJan 24, 2020
@josalem
josalem
deleted the

dev/josalem/diagnostics-socket-name

branch
January 24, 2020 00:27
@josalemjosalem
mentioned this pull request
Feb 8, 2020

Closed

@shirhattishirhatti
mentioned this pull request
Jun 4, 2020

Closed

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


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+2 more reviewers

@mikem8361mikem8361mikem8361 approved these changes
@sywhangsywhangsywhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclrEventPipe

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem@safern@noahfalk@mikem8361@sywhang@Dotnet-GitSync-Bot

, '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
Add COMPlus_DiagnosticsServerTransportPath - #1600
Merged
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Jan 24, 2020
Merged
Add COMPlus_DiagnosticsServerTransportPath#1600
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Conversation
@josalem
@josalemjosalem
commented
Jan 10, 2020

Copy link


Copy Markdown

Contributor
  • allows users to specify a location for the Diagnostics Server
This change allows users to change the location of the Diagnostics Server's OS Transport (Named Pipe on Windows and Unix Domain Socket on Linux).
I'm doing some end-to-end testing with an updated version of the .NET Global Diagnostics Tools that allows connecting to a specific pipe. I have tested this on Windows successfully and will be checking on Linux soon (I'll update this PR with the results).
CC - @tommcdon, @mikem8361
@josalemjosalem
added this to the 5.0 milestone
Jan 10, 2020
@josalemjosalem
self-assigned this
Jan 10, 2020
noahfalk
noahfalk
requested changes
Jan 11, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'm requesting a little refactoring, but I think you are on the right track
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should keep the COMPlus env var handling consolidated in clrconfigvalues.h and then read it with standard accessor (CLRConfig::GetConfigValue). You can pass in the alternate name as a string parameter and read it using code in the VM, for example in DiagnosticServer::Initialize() right next to where we read the EnableDiagnostics variable.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I refactored it into DiagnosticServer::Initialize.
Comment threadsrc/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Same deal for the env var
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It isn't ideal that it will be an either/or choice between connecting via PID or connecting via a custom path. I think it is a reasonable place to start though and we can wait for feedback. The ideal (which would take a good bit more work) is to listen to both the custom and default pipes simultaneously rather forcing a choice between them.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I agree. I'll look into what it would take to do this. I imagine we could just create both sockets and then use a select like construct to call accept on both.
@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalemjosalem
mentioned this pull request
Jan 13, 2020

Closed

sywhang
sywhang
approved these changes
Jan 14, 2020
View reviewed changes
@sywhangsywhang
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.

LGTM
noahfalk
noahfalk
approved these changes
Jan 14, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM 👍
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
cac3baa to
e7dd773CompareJanuary 15, 2020 22:42
@josalem
josalem
commented
Jan 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebasing onto master in an attempt to get tests running. For some reason Windows CoreCLR and Libraries tests weren't running. Or at least the test logs weren't getting generated, which caused the python reporter scripts to fail with no test logs.
@josalemjosalem
mentioned this pull request
Jan 16, 2020

Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
e7dd773 to
894b3d6CompareJanuary 16, 2020 01:20
@josalem
josalem
commented
Jan 16, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing and reopening to use newest pipeline
@josalemjosalem
closed this
Jan 16, 2020
@josalemjosalem
reopened this
Jan 16, 2020
@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020

Closed

@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020
 Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
b2f7a5b to
68bf134CompareJanuary 22, 2020 22:48
@josalemjosalem
closed this
Jan 22, 2020
@josalemjosalem
reopened this
Jan 22, 2020
@josalem
josalem
commented
Jan 23, 2020

Copy link


Copy Markdown

ContributorAuthor
I believe the remaining errors are #1089 since I kicked off the build around 3:30pm and 4pm PST is midnight UST. Will rerun with azp.
/azp run runtime
@safern
safern
commented
Jan 23, 2020

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 23, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalem
josalem
merged commit fd5f7a7
into
dotnet:masterJan 24, 2020
@josalem
josalem
deleted the

dev/josalem/diagnostics-socket-name

branch
January 24, 2020 00:27
@josalemjosalem
mentioned this pull request
Feb 8, 2020

Closed

@shirhattishirhatti
mentioned this pull request
Jun 4, 2020

Closed

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


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+2 more reviewers

@mikem8361mikem8361mikem8361 approved these changes
@sywhangsywhangsywhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclrEventPipe

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem@safern@noahfalk@mikem8361@sywhang@Dotnet-GitSync-Bot

, '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
Add COMPlus_DiagnosticsServerTransportPath - #1600
Merged
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Jan 24, 2020
Merged
Add COMPlus_DiagnosticsServerTransportPath#1600
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Conversation
@josalem
@josalemjosalem
commented
Jan 10, 2020

Copy link


Copy Markdown

Contributor
  • allows users to specify a location for the Diagnostics Server
This change allows users to change the location of the Diagnostics Server's OS Transport (Named Pipe on Windows and Unix Domain Socket on Linux).
I'm doing some end-to-end testing with an updated version of the .NET Global Diagnostics Tools that allows connecting to a specific pipe. I have tested this on Windows successfully and will be checking on Linux soon (I'll update this PR with the results).
CC - @tommcdon, @mikem8361
@josalemjosalem
added this to the 5.0 milestone
Jan 10, 2020
@josalemjosalem
self-assigned this
Jan 10, 2020
noahfalk
noahfalk
requested changes
Jan 11, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'm requesting a little refactoring, but I think you are on the right track
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should keep the COMPlus env var handling consolidated in clrconfigvalues.h and then read it with standard accessor (CLRConfig::GetConfigValue). You can pass in the alternate name as a string parameter and read it using code in the VM, for example in DiagnosticServer::Initialize() right next to where we read the EnableDiagnostics variable.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I refactored it into DiagnosticServer::Initialize.
Comment threadsrc/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Same deal for the env var
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It isn't ideal that it will be an either/or choice between connecting via PID or connecting via a custom path. I think it is a reasonable place to start though and we can wait for feedback. The ideal (which would take a good bit more work) is to listen to both the custom and default pipes simultaneously rather forcing a choice between them.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I agree. I'll look into what it would take to do this. I imagine we could just create both sockets and then use a select like construct to call accept on both.
@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalemjosalem
mentioned this pull request
Jan 13, 2020

Closed

sywhang
sywhang
approved these changes
Jan 14, 2020
View reviewed changes
@sywhangsywhang
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.

LGTM
noahfalk
noahfalk
approved these changes
Jan 14, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM 👍
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
cac3baa to
e7dd773CompareJanuary 15, 2020 22:42
@josalem
josalem
commented
Jan 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebasing onto master in an attempt to get tests running. For some reason Windows CoreCLR and Libraries tests weren't running. Or at least the test logs weren't getting generated, which caused the python reporter scripts to fail with no test logs.
@josalemjosalem
mentioned this pull request
Jan 16, 2020

Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
e7dd773 to
894b3d6CompareJanuary 16, 2020 01:20
@josalem
josalem
commented
Jan 16, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing and reopening to use newest pipeline
@josalemjosalem
closed this
Jan 16, 2020
@josalemjosalem
reopened this
Jan 16, 2020
@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020

Closed

@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020
 Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
b2f7a5b to
68bf134CompareJanuary 22, 2020 22:48
@josalemjosalem
closed this
Jan 22, 2020
@josalemjosalem
reopened this
Jan 22, 2020
@josalem
josalem
commented
Jan 23, 2020

Copy link


Copy Markdown

ContributorAuthor
I believe the remaining errors are #1089 since I kicked off the build around 3:30pm and 4pm PST is midnight UST. Will rerun with azp.
/azp run runtime
@safern
safern
commented
Jan 23, 2020

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 23, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalem
josalem
merged commit fd5f7a7
into
dotnet:masterJan 24, 2020
@josalem
josalem
deleted the

dev/josalem/diagnostics-socket-name

branch
January 24, 2020 00:27
@josalemjosalem
mentioned this pull request
Feb 8, 2020

Closed

@shirhattishirhatti
mentioned this pull request
Jun 4, 2020

Closed

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


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+2 more reviewers

@mikem8361mikem8361mikem8361 approved these changes
@sywhangsywhangsywhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclrEventPipe

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem@safern@noahfalk@mikem8361@sywhang@Dotnet-GitSync-Bot

, '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
Add COMPlus_DiagnosticsServerTransportPath - #1600
Merged
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Jan 24, 2020
Merged
Add COMPlus_DiagnosticsServerTransportPath#1600
josalem merged 4 commits into
dotnet:masterfrom 
josalem:dev/josalem/diagnostics-socket-name
Conversation
@josalem
@josalemjosalem
commented
Jan 10, 2020

Copy link


Copy Markdown

Contributor
  • allows users to specify a location for the Diagnostics Server
This change allows users to change the location of the Diagnostics Server's OS Transport (Named Pipe on Windows and Unix Domain Socket on Linux).
I'm doing some end-to-end testing with an updated version of the .NET Global Diagnostics Tools that allows connecting to a specific pipe. I have tested this on Windows successfully and will be checking on Linux soon (I'll update this PR with the results).
CC - @tommcdon, @mikem8361
@josalemjosalem
added this to the 5.0 milestone
Jan 10, 2020
@josalemjosalem
self-assigned this
Jan 10, 2020
noahfalk
noahfalk
requested changes
Jan 11, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'm requesting a little refactoring, but I think you are on the right track
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We should keep the COMPlus env var handling consolidated in clrconfigvalues.h and then read it with standard accessor (CLRConfig::GetConfigValue). You can pass in the alternate name as a string parameter and read it using code in the VM, for example in DiagnosticServer::Initialize() right next to where we read the EnableDiagnostics variable.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I refactored it into DiagnosticServer::Initialize.
Comment threadsrc/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Same deal for the env var
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 11, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It isn't ideal that it will be an either/or choice between connecting via PID or connecting via a custom path. I think it is a reasonable place to start though and we can wait for feedback. The ideal (which would take a good bit more work) is to listen to both the custom and default pipes simultaneously rather forcing a choice between them.
@josalemjosalemJan 14, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I agree. I'll look into what it would take to do this. I imagine we could just create both sockets and then use a select like construct to call accept on both.
@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalemjosalem
mentioned this pull request
Jan 13, 2020

Closed

sywhang
sywhang
approved these changes
Jan 14, 2020
View reviewed changes
@sywhangsywhang
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.

LGTM
noahfalk
noahfalk
approved these changes
Jan 14, 2020
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM 👍
Comment threadsrc/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp
Outdated

@noahfalknoahfalkJan 14, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'll look into what it would take to do this
Don't spend too much time on it : ) I just mentioned it so everyone was aware, not a request that we implement it right now.
@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
cac3baa to
e7dd773CompareJanuary 15, 2020 22:42
@josalem
josalem
commented
Jan 15, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebasing onto master in an attempt to get tests running. For some reason Windows CoreCLR and Libraries tests weren't running. Or at least the test logs weren't getting generated, which caused the python reporter scripts to fail with no test logs.
@josalemjosalem
mentioned this pull request
Jan 16, 2020

Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
e7dd773 to
894b3d6CompareJanuary 16, 2020 01:20
@josalem
josalem
commented
Jan 16, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing and reopening to use newest pipeline
@josalemjosalem
closed this
Jan 16, 2020
@josalemjosalem
reopened this
Jan 16, 2020
@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020

Closed

@josalem
josalem
commented
Jan 22, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 22, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalemjosalem
mentioned this pull request
Jan 22, 2020
 Closed

@josalem
josalemforce-pushed
the
dev/josalem/diagnostics-socket-name
branch
from
b2f7a5b to
68bf134CompareJanuary 22, 2020 22:48
@josalemjosalem
closed this
Jan 22, 2020
@josalemjosalem
reopened this
Jan 22, 2020
@josalem
josalem
commented
Jan 23, 2020

Copy link


Copy Markdown

ContributorAuthor
I believe the remaining errors are #1089 since I kicked off the build around 3:30pm and 4pm PST is midnight UST. Will rerun with azp.
/azp run runtime
@safern
safern
commented
Jan 23, 2020

Copy link


Copy Markdown

Member
/azp run runtime
@azure-pipelines
azure-pipelinesBot
commented
Jan 23, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@josalem
josalem
merged commit fd5f7a7
into
dotnet:masterJan 24, 2020
@josalem
josalem
deleted the

dev/josalem/diagnostics-socket-name

branch
January 24, 2020 00:27
@josalemjosalem
mentioned this pull request
Feb 8, 2020

Closed

@shirhattishirhatti
mentioned this pull request
Jun 4, 2020

Closed

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


Reviewers

@noahfalknoahfalknoahfalk approved these changes

+2 more reviewers

@mikem8361mikem8361mikem8361 approved these changes
@sywhangsywhangsywhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@josalemjosalem

Labels

area-Diagnostics-coreclrEventPipe

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem@safern@noahfalk@mikem8361@sywhang@Dotnet-GitSync-Bot