Skip to content

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default - #75248

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config
Oct 4, 2022
Merged

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default#75248
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config

Conversation

@davmason

Copy link
Copy Markdown
Contributor

Fixesdotnet/diagnostics#3298

EventSource uses bits 44-47 of the keywords to store data about sessions:

internalconstintSHIFT_SESSION_TO_KEYWORD=44;// bits 44-47 inclusive are reserved
internalconstuintMASK=0x0fU;// the mask of 4 reserved bits
internalconstuintMAX=4;// maximum number of simultaneous ETW sessions supported

And we set them all to 1 by default in the manifest:
metadata.Descriptor=newEventDescriptor(
eventAttribute.EventId,
eventAttribute.Version,
#if FEATURE_MANAGED_ETW_CHANNELS
(byte)eventAttribute.Channel,
#else
(byte)0,
#endif
(byte)eventAttribute.Level,
(byte)eventAttribute.Opcode,
(int)eventAttribute.Task,
unchecked((long)((ulong)eventAttribute.Keywords|SessionMask.All.ToEventKeywords())));

This means that when we compute whether an event should be enabled we won't turn on EventSource events by default, since the event keywords are non-zero 0xF00000000000:

boolkeyword_enabled= (keywords==0) || ((session_keyword&keywords) !=0);

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091, but any other way of enabling EventPipe (env vars, other clients, etc) will still have this issue.

This fix lets the default EventSource keyword act as if it were 0 for all sessions.

This does have two compat issues

  • If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set
  • We cannot use those keywords for runtime events, our current high bit is 42 0x20000000000 so we are very close to that number. I don't think we have a choice regardless of this change though, due to EventSource treating those bits as reserved.

@davmasondavmason added this to the 8.0.0 milestone Sep 8, 2022
@davmason
davmason requested review from a team and lateralusXSeptember 8, 2022 08:52
@davmasondavmason self-assigned this Sep 8, 2022

@lateralusXlateralusX 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/native/eventpipe/ep-provider.c Outdated
int64_t session_keyword = ep_session_provider_get_keywords (session_provider);
EventPipeEventLevel session_level = ep_session_provider_get_logging_level (session_provider);
// EventSources always set 0xF00000000000 to signify no keywords
int64_t session_mask = ~0xF00000000000;

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.

Rather than do this filtering at the point of use, how about we filter these keyword bits at the point we are initializing the event in provider_add_event?

@noahfalk

Copy link
Copy Markdown
Member

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091

Rats, I missed when this change went in. I may be unaware of constraints Sung was under when he did that, but this change you have here looks like a much better approach.

@noahfalk

Copy link
Copy Markdown
Member

If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set

If they did that they would already be seeing weird behavior under ETW. Its possible someone does it and never uses their EventSource with ETW, but the odds of that seem low enough that I am willing to risk inconveniencing them.

@runfoapprunfoappBot mentioned this pull request Sep 21, 2022
@mikelle-rogers

Copy link
Copy Markdown
Member

Is there documentation that needs to be updated with this change?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@mikelle-rogers no documentation needed, this is making things work as already documented

@davmasondavmason reopened this Sep 30, 2022
@davmason

Copy link
Copy Markdown
ContributorAuthor

I found out via failing tests that -1 as a keyword is special and we can't modify it, so I added that check

@davmason
davmason merged commit 28c7eb8 into dotnet:mainOct 4, 2022
@adamsitnik

Copy link
Copy Markdown
Member

@davmason are you going to backport this PR to 7 and 6?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@adamsitnik do you know of any customers that are asking for it? I'm happy to go through the process but the servicing bar requires a customer blocked on it

@adamsitnik

Copy link
Copy Markdown
Member

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

do you know of any customers that are asking for it?

@tmds ?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

Yes, that looks like the same issue. Trying to collect EventSources using the environment variables is broken. You can work around by either using dotnet-trace to collect instead, or setting keywords to 0xFFFFFFFF for any EventSources you want events from.

If we have people asking for the fix just point them to me and I can start the servicing process

@adamsitnik

Copy link
Copy Markdown
Member

Yes, that looks like the same issue.
If we have people asking for the fix just point them to me and I can start the servicing process

Then this would be me as I want to publish a new BenchmarkDotNet version with the PerfCollectDiagnoser (dotnet/BenchmarkDotNet#2117). It's a plugin that does allow for profiling with perfcollect during the benchmarking (we already have sth like that for EventPipe, but it's missing the native call stack info which is often important).

I need the BDN events to be able to tell when given benchmark iteration started and finished to be able to implement detection of regression on top of it. This year I am aiming at adding a BDN feature where the user just specifies the benchmark that has regressed between .NET releases, BDN runs it with profiler attached, loads the trace file using Trace Event, filters it based on the events to selected time frame and then uses Trace Event to diff the traces and just tell the user where the regression is.

@davmason

Copy link
Copy Markdown
ContributorAuthor

I'm happy to take it to servicing if that is the best outcome, but the next servicing date is in January so no fixes would be available until then. I suspect it will work better for you to find a workaround

Are you able to use the diagnostics client instead of the environment variables? https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

There is already a workaround in that library and you wouldn't have to wait on any servicing fixes, the client and runtime support attaching to your own process and attaching at startup, so it should work for most scenarios

davmason added a commit to davmason/runtime that referenced this pull request Nov 2, 2022
carlossanlop pushed a commit that referenced this pull request Nov 7, 2022
…Pipe sessions (#75248) (#77811)
* Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default (#75248)
Update ep-provider.c
* Update ep-provider.c
* Update src/native/eventpipe/ep-provider.c
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set providers using EventPipeConfig

5 participants

@davmason@noahfalk@mikelle-rogers@adamsitnik@lateralusX
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default by davmason · Pull Request #75248 · dotnet/runtime · GitHub
Skip to content

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default - #75248

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config
Oct 4, 2022
Merged

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default#75248
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config

Conversation

@davmason

Copy link
Copy Markdown
Contributor

Fixesdotnet/diagnostics#3298

EventSource uses bits 44-47 of the keywords to store data about sessions:

internalconstintSHIFT_SESSION_TO_KEYWORD=44;// bits 44-47 inclusive are reserved
internalconstuintMASK=0x0fU;// the mask of 4 reserved bits
internalconstuintMAX=4;// maximum number of simultaneous ETW sessions supported

And we set them all to 1 by default in the manifest:
metadata.Descriptor=newEventDescriptor(
eventAttribute.EventId,
eventAttribute.Version,
#if FEATURE_MANAGED_ETW_CHANNELS
(byte)eventAttribute.Channel,
#else
(byte)0,
#endif
(byte)eventAttribute.Level,
(byte)eventAttribute.Opcode,
(int)eventAttribute.Task,
unchecked((long)((ulong)eventAttribute.Keywords|SessionMask.All.ToEventKeywords())));

This means that when we compute whether an event should be enabled we won't turn on EventSource events by default, since the event keywords are non-zero 0xF00000000000:

boolkeyword_enabled= (keywords==0) || ((session_keyword&keywords) !=0);

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091, but any other way of enabling EventPipe (env vars, other clients, etc) will still have this issue.

This fix lets the default EventSource keyword act as if it were 0 for all sessions.

This does have two compat issues

  • If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set
  • We cannot use those keywords for runtime events, our current high bit is 42 0x20000000000 so we are very close to that number. I don't think we have a choice regardless of this change though, due to EventSource treating those bits as reserved.

@davmasondavmason added this to the 8.0.0 milestone Sep 8, 2022
@davmason
davmason requested review from a team and lateralusXSeptember 8, 2022 08:52
@davmasondavmason self-assigned this Sep 8, 2022

@lateralusXlateralusX 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/native/eventpipe/ep-provider.c Outdated
int64_t session_keyword = ep_session_provider_get_keywords (session_provider);
EventPipeEventLevel session_level = ep_session_provider_get_logging_level (session_provider);
// EventSources always set 0xF00000000000 to signify no keywords
int64_t session_mask = ~0xF00000000000;

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.

Rather than do this filtering at the point of use, how about we filter these keyword bits at the point we are initializing the event in provider_add_event?

@noahfalk

Copy link
Copy Markdown
Member

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091

Rats, I missed when this change went in. I may be unaware of constraints Sung was under when he did that, but this change you have here looks like a much better approach.

@noahfalk

Copy link
Copy Markdown
Member

If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set

If they did that they would already be seeing weird behavior under ETW. Its possible someone does it and never uses their EventSource with ETW, but the odds of that seem low enough that I am willing to risk inconveniencing them.

@runfoapprunfoappBot mentioned this pull request Sep 21, 2022
@mikelle-rogers

Copy link
Copy Markdown
Member

Is there documentation that needs to be updated with this change?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@mikelle-rogers no documentation needed, this is making things work as already documented

@davmasondavmason reopened this Sep 30, 2022
@davmason

Copy link
Copy Markdown
ContributorAuthor

I found out via failing tests that -1 as a keyword is special and we can't modify it, so I added that check

@davmason
davmason merged commit 28c7eb8 into dotnet:mainOct 4, 2022
@adamsitnik

Copy link
Copy Markdown
Member

@davmason are you going to backport this PR to 7 and 6?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@adamsitnik do you know of any customers that are asking for it? I'm happy to go through the process but the servicing bar requires a customer blocked on it

@adamsitnik

Copy link
Copy Markdown
Member

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

do you know of any customers that are asking for it?

@tmds ?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

Yes, that looks like the same issue. Trying to collect EventSources using the environment variables is broken. You can work around by either using dotnet-trace to collect instead, or setting keywords to 0xFFFFFFFF for any EventSources you want events from.

If we have people asking for the fix just point them to me and I can start the servicing process

@adamsitnik

Copy link
Copy Markdown
Member

Yes, that looks like the same issue.
If we have people asking for the fix just point them to me and I can start the servicing process

Then this would be me as I want to publish a new BenchmarkDotNet version with the PerfCollectDiagnoser (dotnet/BenchmarkDotNet#2117). It's a plugin that does allow for profiling with perfcollect during the benchmarking (we already have sth like that for EventPipe, but it's missing the native call stack info which is often important).

I need the BDN events to be able to tell when given benchmark iteration started and finished to be able to implement detection of regression on top of it. This year I am aiming at adding a BDN feature where the user just specifies the benchmark that has regressed between .NET releases, BDN runs it with profiler attached, loads the trace file using Trace Event, filters it based on the events to selected time frame and then uses Trace Event to diff the traces and just tell the user where the regression is.

@davmason

Copy link
Copy Markdown
ContributorAuthor

I'm happy to take it to servicing if that is the best outcome, but the next servicing date is in January so no fixes would be available until then. I suspect it will work better for you to find a workaround

Are you able to use the diagnostics client instead of the environment variables? https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

There is already a workaround in that library and you wouldn't have to wait on any servicing fixes, the client and runtime support attaching to your own process and attaching at startup, so it should work for most scenarios

davmason added a commit to davmason/runtime that referenced this pull request Nov 2, 2022
carlossanlop pushed a commit that referenced this pull request Nov 7, 2022
…Pipe sessions (#75248) (#77811)
* Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default (#75248)
Update ep-provider.c
* Update ep-provider.c
* Update src/native/eventpipe/ep-provider.c
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set providers using EventPipeConfig

5 participants

@davmason@noahfalk@mikelle-rogers@adamsitnik@lateralusX
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default by davmason · Pull Request #75248 · dotnet/runtime · GitHub
Skip to content

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default - #75248

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config
Oct 4, 2022
Merged

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default#75248
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config

Conversation

@davmason

Copy link
Copy Markdown
Contributor

Fixesdotnet/diagnostics#3298

EventSource uses bits 44-47 of the keywords to store data about sessions:

internalconstintSHIFT_SESSION_TO_KEYWORD=44;// bits 44-47 inclusive are reserved
internalconstuintMASK=0x0fU;// the mask of 4 reserved bits
internalconstuintMAX=4;// maximum number of simultaneous ETW sessions supported

And we set them all to 1 by default in the manifest:
metadata.Descriptor=newEventDescriptor(
eventAttribute.EventId,
eventAttribute.Version,
#if FEATURE_MANAGED_ETW_CHANNELS
(byte)eventAttribute.Channel,
#else
(byte)0,
#endif
(byte)eventAttribute.Level,
(byte)eventAttribute.Opcode,
(int)eventAttribute.Task,
unchecked((long)((ulong)eventAttribute.Keywords|SessionMask.All.ToEventKeywords())));

This means that when we compute whether an event should be enabled we won't turn on EventSource events by default, since the event keywords are non-zero 0xF00000000000:

boolkeyword_enabled= (keywords==0) || ((session_keyword&keywords) !=0);

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091, but any other way of enabling EventPipe (env vars, other clients, etc) will still have this issue.

This fix lets the default EventSource keyword act as if it were 0 for all sessions.

This does have two compat issues

  • If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set
  • We cannot use those keywords for runtime events, our current high bit is 42 0x20000000000 so we are very close to that number. I don't think we have a choice regardless of this change though, due to EventSource treating those bits as reserved.

@davmasondavmason added this to the 8.0.0 milestone Sep 8, 2022
@davmason
davmason requested review from a team and lateralusXSeptember 8, 2022 08:52
@davmasondavmason self-assigned this Sep 8, 2022

@lateralusXlateralusX 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/native/eventpipe/ep-provider.c Outdated
int64_t session_keyword = ep_session_provider_get_keywords (session_provider);
EventPipeEventLevel session_level = ep_session_provider_get_logging_level (session_provider);
// EventSources always set 0xF00000000000 to signify no keywords
int64_t session_mask = ~0xF00000000000;

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.

Rather than do this filtering at the point of use, how about we filter these keyword bits at the point we are initializing the event in provider_add_event?

@noahfalk

Copy link
Copy Markdown
Member

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091

Rats, I missed when this change went in. I may be unaware of constraints Sung was under when he did that, but this change you have here looks like a much better approach.

@noahfalk

Copy link
Copy Markdown
Member

If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set

If they did that they would already be seeing weird behavior under ETW. Its possible someone does it and never uses their EventSource with ETW, but the odds of that seem low enough that I am willing to risk inconveniencing them.

@runfoapprunfoappBot mentioned this pull request Sep 21, 2022
@mikelle-rogers

Copy link
Copy Markdown
Member

Is there documentation that needs to be updated with this change?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@mikelle-rogers no documentation needed, this is making things work as already documented

@davmasondavmason reopened this Sep 30, 2022
@davmason

Copy link
Copy Markdown
ContributorAuthor

I found out via failing tests that -1 as a keyword is special and we can't modify it, so I added that check

@davmason
davmason merged commit 28c7eb8 into dotnet:mainOct 4, 2022
@adamsitnik

Copy link
Copy Markdown
Member

@davmason are you going to backport this PR to 7 and 6?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@adamsitnik do you know of any customers that are asking for it? I'm happy to go through the process but the servicing bar requires a customer blocked on it

@adamsitnik

Copy link
Copy Markdown
Member

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

do you know of any customers that are asking for it?

@tmds ?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

Yes, that looks like the same issue. Trying to collect EventSources using the environment variables is broken. You can work around by either using dotnet-trace to collect instead, or setting keywords to 0xFFFFFFFF for any EventSources you want events from.

If we have people asking for the fix just point them to me and I can start the servicing process

@adamsitnik

Copy link
Copy Markdown
Member

Yes, that looks like the same issue.
If we have people asking for the fix just point them to me and I can start the servicing process

Then this would be me as I want to publish a new BenchmarkDotNet version with the PerfCollectDiagnoser (dotnet/BenchmarkDotNet#2117). It's a plugin that does allow for profiling with perfcollect during the benchmarking (we already have sth like that for EventPipe, but it's missing the native call stack info which is often important).

I need the BDN events to be able to tell when given benchmark iteration started and finished to be able to implement detection of regression on top of it. This year I am aiming at adding a BDN feature where the user just specifies the benchmark that has regressed between .NET releases, BDN runs it with profiler attached, loads the trace file using Trace Event, filters it based on the events to selected time frame and then uses Trace Event to diff the traces and just tell the user where the regression is.

@davmason

Copy link
Copy Markdown
ContributorAuthor

I'm happy to take it to servicing if that is the best outcome, but the next servicing date is in January so no fixes would be available until then. I suspect it will work better for you to find a workaround

Are you able to use the diagnostics client instead of the environment variables? https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

There is already a workaround in that library and you wouldn't have to wait on any servicing fixes, the client and runtime support attaching to your own process and attaching at startup, so it should work for most scenarios

davmason added a commit to davmason/runtime that referenced this pull request Nov 2, 2022
carlossanlop pushed a commit that referenced this pull request Nov 7, 2022
…Pipe sessions (#75248) (#77811)
* Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default (#75248)
Update ep-provider.c
* Update ep-provider.c
* Update src/native/eventpipe/ep-provider.c
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set providers using EventPipeConfig

5 participants

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

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default - #75248

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config
Oct 4, 2022
Merged

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default#75248
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config

Conversation

@davmason

Copy link
Copy Markdown
Contributor

Fixesdotnet/diagnostics#3298

EventSource uses bits 44-47 of the keywords to store data about sessions:

internalconstintSHIFT_SESSION_TO_KEYWORD=44;// bits 44-47 inclusive are reserved
internalconstuintMASK=0x0fU;// the mask of 4 reserved bits
internalconstuintMAX=4;// maximum number of simultaneous ETW sessions supported

And we set them all to 1 by default in the manifest:
metadata.Descriptor=newEventDescriptor(
eventAttribute.EventId,
eventAttribute.Version,
#if FEATURE_MANAGED_ETW_CHANNELS
(byte)eventAttribute.Channel,
#else
(byte)0,
#endif
(byte)eventAttribute.Level,
(byte)eventAttribute.Opcode,
(int)eventAttribute.Task,
unchecked((long)((ulong)eventAttribute.Keywords|SessionMask.All.ToEventKeywords())));

This means that when we compute whether an event should be enabled we won't turn on EventSource events by default, since the event keywords are non-zero 0xF00000000000:

boolkeyword_enabled= (keywords==0) || ((session_keyword&keywords) !=0);

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091, but any other way of enabling EventPipe (env vars, other clients, etc) will still have this issue.

This fix lets the default EventSource keyword act as if it were 0 for all sessions.

This does have two compat issues

  • If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set
  • We cannot use those keywords for runtime events, our current high bit is 42 0x20000000000 so we are very close to that number. I don't think we have a choice regardless of this change though, due to EventSource treating those bits as reserved.

@davmasondavmason added this to the 8.0.0 milestone Sep 8, 2022
@davmason
davmason requested review from a team and lateralusXSeptember 8, 2022 08:52
@davmasondavmason self-assigned this Sep 8, 2022

@lateralusXlateralusX 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/native/eventpipe/ep-provider.c Outdated
int64_t session_keyword = ep_session_provider_get_keywords (session_provider);
EventPipeEventLevel session_level = ep_session_provider_get_logging_level (session_provider);
// EventSources always set 0xF00000000000 to signify no keywords
int64_t session_mask = ~0xF00000000000;

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.

Rather than do this filtering at the point of use, how about we filter these keyword bits at the point we are initializing the event in provider_add_event?

@noahfalk

Copy link
Copy Markdown
Member

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091

Rats, I missed when this change went in. I may be unaware of constraints Sung was under when he did that, but this change you have here looks like a much better approach.

@noahfalk

Copy link
Copy Markdown
Member

If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set

If they did that they would already be seeing weird behavior under ETW. Its possible someone does it and never uses their EventSource with ETW, but the odds of that seem low enough that I am willing to risk inconveniencing them.

@runfoapprunfoappBot mentioned this pull request Sep 21, 2022
@mikelle-rogers

Copy link
Copy Markdown
Member

Is there documentation that needs to be updated with this change?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@mikelle-rogers no documentation needed, this is making things work as already documented

@davmasondavmason reopened this Sep 30, 2022
@davmason

Copy link
Copy Markdown
ContributorAuthor

I found out via failing tests that -1 as a keyword is special and we can't modify it, so I added that check

@davmason
davmason merged commit 28c7eb8 into dotnet:mainOct 4, 2022
@adamsitnik

Copy link
Copy Markdown
Member

@davmason are you going to backport this PR to 7 and 6?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@adamsitnik do you know of any customers that are asking for it? I'm happy to go through the process but the servicing bar requires a customer blocked on it

@adamsitnik

Copy link
Copy Markdown
Member

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

do you know of any customers that are asking for it?

@tmds ?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

Yes, that looks like the same issue. Trying to collect EventSources using the environment variables is broken. You can work around by either using dotnet-trace to collect instead, or setting keywords to 0xFFFFFFFF for any EventSources you want events from.

If we have people asking for the fix just point them to me and I can start the servicing process

@adamsitnik

Copy link
Copy Markdown
Member

Yes, that looks like the same issue.
If we have people asking for the fix just point them to me and I can start the servicing process

Then this would be me as I want to publish a new BenchmarkDotNet version with the PerfCollectDiagnoser (dotnet/BenchmarkDotNet#2117). It's a plugin that does allow for profiling with perfcollect during the benchmarking (we already have sth like that for EventPipe, but it's missing the native call stack info which is often important).

I need the BDN events to be able to tell when given benchmark iteration started and finished to be able to implement detection of regression on top of it. This year I am aiming at adding a BDN feature where the user just specifies the benchmark that has regressed between .NET releases, BDN runs it with profiler attached, loads the trace file using Trace Event, filters it based on the events to selected time frame and then uses Trace Event to diff the traces and just tell the user where the regression is.

@davmason

Copy link
Copy Markdown
ContributorAuthor

I'm happy to take it to servicing if that is the best outcome, but the next servicing date is in January so no fixes would be available until then. I suspect it will work better for you to find a workaround

Are you able to use the diagnostics client instead of the environment variables? https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

There is already a workaround in that library and you wouldn't have to wait on any servicing fixes, the client and runtime support attaching to your own process and attaching at startup, so it should work for most scenarios

davmason added a commit to davmason/runtime that referenced this pull request Nov 2, 2022
carlossanlop pushed a commit that referenced this pull request Nov 7, 2022
…Pipe sessions (#75248) (#77811)
* Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default (#75248)
Update ep-provider.c
* Update ep-provider.c
* Update src/native/eventpipe/ep-provider.c
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set providers using EventPipeConfig

5 participants

@davmason@noahfalk@mikelle-rogers@adamsitnik@lateralusX
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default by davmason · Pull Request #75248 · dotnet/runtime · GitHub
Skip to content

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default - #75248

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config
Oct 4, 2022
Merged

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default#75248
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config

Conversation

@davmason

Copy link
Copy Markdown
Contributor

Fixesdotnet/diagnostics#3298

EventSource uses bits 44-47 of the keywords to store data about sessions:

internalconstintSHIFT_SESSION_TO_KEYWORD=44;// bits 44-47 inclusive are reserved
internalconstuintMASK=0x0fU;// the mask of 4 reserved bits
internalconstuintMAX=4;// maximum number of simultaneous ETW sessions supported

And we set them all to 1 by default in the manifest:
metadata.Descriptor=newEventDescriptor(
eventAttribute.EventId,
eventAttribute.Version,
#if FEATURE_MANAGED_ETW_CHANNELS
(byte)eventAttribute.Channel,
#else
(byte)0,
#endif
(byte)eventAttribute.Level,
(byte)eventAttribute.Opcode,
(int)eventAttribute.Task,
unchecked((long)((ulong)eventAttribute.Keywords|SessionMask.All.ToEventKeywords())));

This means that when we compute whether an event should be enabled we won't turn on EventSource events by default, since the event keywords are non-zero 0xF00000000000:

boolkeyword_enabled= (keywords==0) || ((session_keyword&keywords) !=0);

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091, but any other way of enabling EventPipe (env vars, other clients, etc) will still have this issue.

This fix lets the default EventSource keyword act as if it were 0 for all sessions.

This does have two compat issues

  • If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set
  • We cannot use those keywords for runtime events, our current high bit is 42 0x20000000000 so we are very close to that number. I don't think we have a choice regardless of this change though, due to EventSource treating those bits as reserved.

@davmasondavmason added this to the 8.0.0 milestone Sep 8, 2022
@davmason
davmason requested review from a team and lateralusXSeptember 8, 2022 08:52
@davmasondavmason self-assigned this Sep 8, 2022

@lateralusXlateralusX 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/native/eventpipe/ep-provider.c Outdated
int64_t session_keyword = ep_session_provider_get_keywords (session_provider);
EventPipeEventLevel session_level = ep_session_provider_get_logging_level (session_provider);
// EventSources always set 0xF00000000000 to signify no keywords
int64_t session_mask = ~0xF00000000000;

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.

Rather than do this filtering at the point of use, how about we filter these keyword bits at the point we are initializing the event in provider_add_event?

@noahfalk

Copy link
Copy Markdown
Member

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091

Rats, I missed when this change went in. I may be unaware of constraints Sung was under when he did that, but this change you have here looks like a much better approach.

@noahfalk

Copy link
Copy Markdown
Member

If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set

If they did that they would already be seeing weird behavior under ETW. Its possible someone does it and never uses their EventSource with ETW, but the odds of that seem low enough that I am willing to risk inconveniencing them.

@runfoapprunfoappBot mentioned this pull request Sep 21, 2022
@mikelle-rogers

Copy link
Copy Markdown
Member

Is there documentation that needs to be updated with this change?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@mikelle-rogers no documentation needed, this is making things work as already documented

@davmasondavmason reopened this Sep 30, 2022
@davmason

Copy link
Copy Markdown
ContributorAuthor

I found out via failing tests that -1 as a keyword is special and we can't modify it, so I added that check

@davmason
davmason merged commit 28c7eb8 into dotnet:mainOct 4, 2022
@adamsitnik

Copy link
Copy Markdown
Member

@davmason are you going to backport this PR to 7 and 6?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@adamsitnik do you know of any customers that are asking for it? I'm happy to go through the process but the servicing bar requires a customer blocked on it

@adamsitnik

Copy link
Copy Markdown
Member

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

do you know of any customers that are asking for it?

@tmds ?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

Yes, that looks like the same issue. Trying to collect EventSources using the environment variables is broken. You can work around by either using dotnet-trace to collect instead, or setting keywords to 0xFFFFFFFF for any EventSources you want events from.

If we have people asking for the fix just point them to me and I can start the servicing process

@adamsitnik

Copy link
Copy Markdown
Member

Yes, that looks like the same issue.
If we have people asking for the fix just point them to me and I can start the servicing process

Then this would be me as I want to publish a new BenchmarkDotNet version with the PerfCollectDiagnoser (dotnet/BenchmarkDotNet#2117). It's a plugin that does allow for profiling with perfcollect during the benchmarking (we already have sth like that for EventPipe, but it's missing the native call stack info which is often important).

I need the BDN events to be able to tell when given benchmark iteration started and finished to be able to implement detection of regression on top of it. This year I am aiming at adding a BDN feature where the user just specifies the benchmark that has regressed between .NET releases, BDN runs it with profiler attached, loads the trace file using Trace Event, filters it based on the events to selected time frame and then uses Trace Event to diff the traces and just tell the user where the regression is.

@davmason

Copy link
Copy Markdown
ContributorAuthor

I'm happy to take it to servicing if that is the best outcome, but the next servicing date is in January so no fixes would be available until then. I suspect it will work better for you to find a workaround

Are you able to use the diagnostics client instead of the environment variables? https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

There is already a workaround in that library and you wouldn't have to wait on any servicing fixes, the client and runtime support attaching to your own process and attaching at startup, so it should work for most scenarios

davmason added a commit to davmason/runtime that referenced this pull request Nov 2, 2022
carlossanlop pushed a commit that referenced this pull request Nov 7, 2022
…Pipe sessions (#75248) (#77811)
* Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default (#75248)
Update ep-provider.c
* Update ep-provider.c
* Update src/native/eventpipe/ep-provider.c
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set providers using EventPipeConfig

5 participants

@davmason@noahfalk@mikelle-rogers@adamsitnik@lateralusX
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default by davmason · Pull Request #75248 · dotnet/runtime · GitHub
Skip to content

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default - #75248

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config
Oct 4, 2022
Merged

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default#75248
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config

Conversation

@davmason

Copy link
Copy Markdown
Contributor

Fixesdotnet/diagnostics#3298

EventSource uses bits 44-47 of the keywords to store data about sessions:

internalconstintSHIFT_SESSION_TO_KEYWORD=44;// bits 44-47 inclusive are reserved
internalconstuintMASK=0x0fU;// the mask of 4 reserved bits
internalconstuintMAX=4;// maximum number of simultaneous ETW sessions supported

And we set them all to 1 by default in the manifest:
metadata.Descriptor=newEventDescriptor(
eventAttribute.EventId,
eventAttribute.Version,
#if FEATURE_MANAGED_ETW_CHANNELS
(byte)eventAttribute.Channel,
#else
(byte)0,
#endif
(byte)eventAttribute.Level,
(byte)eventAttribute.Opcode,
(int)eventAttribute.Task,
unchecked((long)((ulong)eventAttribute.Keywords|SessionMask.All.ToEventKeywords())));

This means that when we compute whether an event should be enabled we won't turn on EventSource events by default, since the event keywords are non-zero 0xF00000000000:

boolkeyword_enabled= (keywords==0) || ((session_keyword&keywords) !=0);

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091, but any other way of enabling EventPipe (env vars, other clients, etc) will still have this issue.

This fix lets the default EventSource keyword act as if it were 0 for all sessions.

This does have two compat issues

  • If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set
  • We cannot use those keywords for runtime events, our current high bit is 42 0x20000000000 so we are very close to that number. I don't think we have a choice regardless of this change though, due to EventSource treating those bits as reserved.

@davmasondavmason added this to the 8.0.0 milestone Sep 8, 2022
@davmason
davmason requested review from a team and lateralusXSeptember 8, 2022 08:52
@davmasondavmason self-assigned this Sep 8, 2022

@lateralusXlateralusX 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/native/eventpipe/ep-provider.c Outdated
int64_t session_keyword = ep_session_provider_get_keywords (session_provider);
EventPipeEventLevel session_level = ep_session_provider_get_logging_level (session_provider);
// EventSources always set 0xF00000000000 to signify no keywords
int64_t session_mask = ~0xF00000000000;

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.

Rather than do this filtering at the point of use, how about we filter these keyword bits at the point we are initializing the event in provider_add_event?

@noahfalk

Copy link
Copy Markdown
Member

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091

Rats, I missed when this change went in. I may be unaware of constraints Sung was under when he did that, but this change you have here looks like a much better approach.

@noahfalk

Copy link
Copy Markdown
Member

If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set

If they did that they would already be seeing weird behavior under ETW. Its possible someone does it and never uses their EventSource with ETW, but the odds of that seem low enough that I am willing to risk inconveniencing them.

@runfoapprunfoappBot mentioned this pull request Sep 21, 2022
@mikelle-rogers

Copy link
Copy Markdown
Member

Is there documentation that needs to be updated with this change?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@mikelle-rogers no documentation needed, this is making things work as already documented

@davmasondavmason reopened this Sep 30, 2022
@davmason

Copy link
Copy Markdown
ContributorAuthor

I found out via failing tests that -1 as a keyword is special and we can't modify it, so I added that check

@davmason
davmason merged commit 28c7eb8 into dotnet:mainOct 4, 2022
@adamsitnik

Copy link
Copy Markdown
Member

@davmason are you going to backport this PR to 7 and 6?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@adamsitnik do you know of any customers that are asking for it? I'm happy to go through the process but the servicing bar requires a customer blocked on it

@adamsitnik

Copy link
Copy Markdown
Member

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

do you know of any customers that are asking for it?

@tmds ?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

Yes, that looks like the same issue. Trying to collect EventSources using the environment variables is broken. You can work around by either using dotnet-trace to collect instead, or setting keywords to 0xFFFFFFFF for any EventSources you want events from.

If we have people asking for the fix just point them to me and I can start the servicing process

@adamsitnik

Copy link
Copy Markdown
Member

Yes, that looks like the same issue.
If we have people asking for the fix just point them to me and I can start the servicing process

Then this would be me as I want to publish a new BenchmarkDotNet version with the PerfCollectDiagnoser (dotnet/BenchmarkDotNet#2117). It's a plugin that does allow for profiling with perfcollect during the benchmarking (we already have sth like that for EventPipe, but it's missing the native call stack info which is often important).

I need the BDN events to be able to tell when given benchmark iteration started and finished to be able to implement detection of regression on top of it. This year I am aiming at adding a BDN feature where the user just specifies the benchmark that has regressed between .NET releases, BDN runs it with profiler attached, loads the trace file using Trace Event, filters it based on the events to selected time frame and then uses Trace Event to diff the traces and just tell the user where the regression is.

@davmason

Copy link
Copy Markdown
ContributorAuthor

I'm happy to take it to servicing if that is the best outcome, but the next servicing date is in January so no fixes would be available until then. I suspect it will work better for you to find a workaround

Are you able to use the diagnostics client instead of the environment variables? https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

There is already a workaround in that library and you wouldn't have to wait on any servicing fixes, the client and runtime support attaching to your own process and attaching at startup, so it should work for most scenarios

davmason added a commit to davmason/runtime that referenced this pull request Nov 2, 2022
carlossanlop pushed a commit that referenced this pull request Nov 7, 2022
…Pipe sessions (#75248) (#77811)
* Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default (#75248)
Update ep-provider.c
* Update ep-provider.c
* Update src/native/eventpipe/ep-provider.c
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set providers using EventPipeConfig

5 participants

@davmason@noahfalk@mikelle-rogers@adamsitnik@lateralusX
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default by davmason · Pull Request #75248 · dotnet/runtime · GitHub
Skip to content

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default - #75248

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config
Oct 4, 2022
Merged

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default#75248
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config

Conversation

@davmason

Copy link
Copy Markdown
Contributor

Fixesdotnet/diagnostics#3298

EventSource uses bits 44-47 of the keywords to store data about sessions:

internalconstintSHIFT_SESSION_TO_KEYWORD=44;// bits 44-47 inclusive are reserved
internalconstuintMASK=0x0fU;// the mask of 4 reserved bits
internalconstuintMAX=4;// maximum number of simultaneous ETW sessions supported

And we set them all to 1 by default in the manifest:
metadata.Descriptor=newEventDescriptor(
eventAttribute.EventId,
eventAttribute.Version,
#if FEATURE_MANAGED_ETW_CHANNELS
(byte)eventAttribute.Channel,
#else
(byte)0,
#endif
(byte)eventAttribute.Level,
(byte)eventAttribute.Opcode,
(int)eventAttribute.Task,
unchecked((long)((ulong)eventAttribute.Keywords|SessionMask.All.ToEventKeywords())));

This means that when we compute whether an event should be enabled we won't turn on EventSource events by default, since the event keywords are non-zero 0xF00000000000:

boolkeyword_enabled= (keywords==0) || ((session_keyword&keywords) !=0);

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091, but any other way of enabling EventPipe (env vars, other clients, etc) will still have this issue.

This fix lets the default EventSource keyword act as if it were 0 for all sessions.

This does have two compat issues

  • If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set
  • We cannot use those keywords for runtime events, our current high bit is 42 0x20000000000 so we are very close to that number. I don't think we have a choice regardless of this change though, due to EventSource treating those bits as reserved.

@davmasondavmason added this to the 8.0.0 milestone Sep 8, 2022
@davmason
davmason requested review from a team and lateralusXSeptember 8, 2022 08:52
@davmasondavmason self-assigned this Sep 8, 2022

@lateralusXlateralusX 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/native/eventpipe/ep-provider.c Outdated
int64_t session_keyword = ep_session_provider_get_keywords (session_provider);
EventPipeEventLevel session_level = ep_session_provider_get_logging_level (session_provider);
// EventSources always set 0xF00000000000 to signify no keywords
int64_t session_mask = ~0xF00000000000;

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.

Rather than do this filtering at the point of use, how about we filter these keyword bits at the point we are initializing the event in provider_add_event?

@noahfalk

Copy link
Copy Markdown
Member

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091

Rats, I missed when this change went in. I may be unaware of constraints Sung was under when he did that, but this change you have here looks like a much better approach.

@noahfalk

Copy link
Copy Markdown
Member

If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set

If they did that they would already be seeing weird behavior under ETW. Its possible someone does it and never uses their EventSource with ETW, but the odds of that seem low enough that I am willing to risk inconveniencing them.

@runfoapprunfoappBot mentioned this pull request Sep 21, 2022
@mikelle-rogers

Copy link
Copy Markdown
Member

Is there documentation that needs to be updated with this change?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@mikelle-rogers no documentation needed, this is making things work as already documented

@davmasondavmason reopened this Sep 30, 2022
@davmason

Copy link
Copy Markdown
ContributorAuthor

I found out via failing tests that -1 as a keyword is special and we can't modify it, so I added that check

@davmason
davmason merged commit 28c7eb8 into dotnet:mainOct 4, 2022
@adamsitnik

Copy link
Copy Markdown
Member

@davmason are you going to backport this PR to 7 and 6?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@adamsitnik do you know of any customers that are asking for it? I'm happy to go through the process but the servicing bar requires a customer blocked on it

@adamsitnik

Copy link
Copy Markdown
Member

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

do you know of any customers that are asking for it?

@tmds ?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

Yes, that looks like the same issue. Trying to collect EventSources using the environment variables is broken. You can work around by either using dotnet-trace to collect instead, or setting keywords to 0xFFFFFFFF for any EventSources you want events from.

If we have people asking for the fix just point them to me and I can start the servicing process

@adamsitnik

Copy link
Copy Markdown
Member

Yes, that looks like the same issue.
If we have people asking for the fix just point them to me and I can start the servicing process

Then this would be me as I want to publish a new BenchmarkDotNet version with the PerfCollectDiagnoser (dotnet/BenchmarkDotNet#2117). It's a plugin that does allow for profiling with perfcollect during the benchmarking (we already have sth like that for EventPipe, but it's missing the native call stack info which is often important).

I need the BDN events to be able to tell when given benchmark iteration started and finished to be able to implement detection of regression on top of it. This year I am aiming at adding a BDN feature where the user just specifies the benchmark that has regressed between .NET releases, BDN runs it with profiler attached, loads the trace file using Trace Event, filters it based on the events to selected time frame and then uses Trace Event to diff the traces and just tell the user where the regression is.

@davmason

Copy link
Copy Markdown
ContributorAuthor

I'm happy to take it to servicing if that is the best outcome, but the next servicing date is in January so no fixes would be available until then. I suspect it will work better for you to find a workaround

Are you able to use the diagnostics client instead of the environment variables? https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

There is already a workaround in that library and you wouldn't have to wait on any servicing fixes, the client and runtime support attaching to your own process and attaching at startup, so it should work for most scenarios

davmason added a commit to davmason/runtime that referenced this pull request Nov 2, 2022
carlossanlop pushed a commit that referenced this pull request Nov 7, 2022
…Pipe sessions (#75248) (#77811)
* Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default (#75248)
Update ep-provider.c
* Update ep-provider.c
* Update src/native/eventpipe/ep-provider.c
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set providers using EventPipeConfig

5 participants

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

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default - #75248

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config
Oct 4, 2022
Merged

Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default#75248
davmason merged 3 commits into
dotnet:mainfrom
davmason:eventsource_config

Conversation

@davmason

Copy link
Copy Markdown
Contributor

Fixesdotnet/diagnostics#3298

EventSource uses bits 44-47 of the keywords to store data about sessions:

internalconstintSHIFT_SESSION_TO_KEYWORD=44;// bits 44-47 inclusive are reserved
internalconstuintMASK=0x0fU;// the mask of 4 reserved bits
internalconstuintMAX=4;// maximum number of simultaneous ETW sessions supported

And we set them all to 1 by default in the manifest:
metadata.Descriptor=newEventDescriptor(
eventAttribute.EventId,
eventAttribute.Version,
#if FEATURE_MANAGED_ETW_CHANNELS
(byte)eventAttribute.Channel,
#else
(byte)0,
#endif
(byte)eventAttribute.Level,
(byte)eventAttribute.Opcode,
(int)eventAttribute.Task,
unchecked((long)((ulong)eventAttribute.Keywords|SessionMask.All.ToEventKeywords())));

This means that when we compute whether an event should be enabled we won't turn on EventSource events by default, since the event keywords are non-zero 0xF00000000000:

boolkeyword_enabled= (keywords==0) || ((session_keyword&keywords) !=0);

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091, but any other way of enabling EventPipe (env vars, other clients, etc) will still have this issue.

This fix lets the default EventSource keyword act as if it were 0 for all sessions.

This does have two compat issues

  • If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set
  • We cannot use those keywords for runtime events, our current high bit is 42 0x20000000000 so we are very close to that number. I don't think we have a choice regardless of this change though, due to EventSource treating those bits as reserved.

@davmasondavmason added this to the 8.0.0 milestone Sep 8, 2022
@davmason
davmason requested review from a team and lateralusXSeptember 8, 2022 08:52
@davmasondavmason self-assigned this Sep 8, 2022

@lateralusXlateralusX 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/native/eventpipe/ep-provider.c Outdated
int64_t session_keyword = ep_session_provider_get_keywords (session_provider);
EventPipeEventLevel session_level = ep_session_provider_get_logging_level (session_provider);
// EventSources always set 0xF00000000000 to signify no keywords
int64_t session_mask = ~0xF00000000000;

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.

Rather than do this filtering at the point of use, how about we filter these keyword bits at the point we are initializing the event in provider_add_event?

@noahfalk

Copy link
Copy Markdown
Member

We have a workaround in the DiagnosticClient introduced here: dotnet/diagnostics#1091

Rats, I missed when this change went in. I may be unaware of constraints Sung was under when he did that, but this change you have here looks like a much better approach.

@noahfalk

Copy link
Copy Markdown
Member

If someone has a provider that was using any of the masked bits it would now start to show up in traces that didn't have the keyword set

If they did that they would already be seeing weird behavior under ETW. Its possible someone does it and never uses their EventSource with ETW, but the odds of that seem low enough that I am willing to risk inconveniencing them.

@runfoapprunfoappBot mentioned this pull request Sep 21, 2022
@mikelle-rogers

Copy link
Copy Markdown
Member

Is there documentation that needs to be updated with this change?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@mikelle-rogers no documentation needed, this is making things work as already documented

@davmasondavmason reopened this Sep 30, 2022
@davmason

Copy link
Copy Markdown
ContributorAuthor

I found out via failing tests that -1 as a keyword is special and we can't modify it, so I added that check

@davmason
davmason merged commit 28c7eb8 into dotnet:mainOct 4, 2022
@adamsitnik

Copy link
Copy Markdown
Member

@davmason are you going to backport this PR to 7 and 6?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@adamsitnik do you know of any customers that are asking for it? I'm happy to go through the process but the servicing bar requires a customer blocked on it

@adamsitnik

Copy link
Copy Markdown
Member

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

do you know of any customers that are asking for it?

@tmds ?

@davmason

Copy link
Copy Markdown
ContributorAuthor

@davmason From my perspective I wonder if this related to microsoft/perfview#1718

Yes, that looks like the same issue. Trying to collect EventSources using the environment variables is broken. You can work around by either using dotnet-trace to collect instead, or setting keywords to 0xFFFFFFFF for any EventSources you want events from.

If we have people asking for the fix just point them to me and I can start the servicing process

@adamsitnik

Copy link
Copy Markdown
Member

Yes, that looks like the same issue.
If we have people asking for the fix just point them to me and I can start the servicing process

Then this would be me as I want to publish a new BenchmarkDotNet version with the PerfCollectDiagnoser (dotnet/BenchmarkDotNet#2117). It's a plugin that does allow for profiling with perfcollect during the benchmarking (we already have sth like that for EventPipe, but it's missing the native call stack info which is often important).

I need the BDN events to be able to tell when given benchmark iteration started and finished to be able to implement detection of regression on top of it. This year I am aiming at adding a BDN feature where the user just specifies the benchmark that has regressed between .NET releases, BDN runs it with profiler attached, loads the trace file using Trace Event, filters it based on the events to selected time frame and then uses Trace Event to diff the traces and just tell the user where the regression is.

@davmason

Copy link
Copy Markdown
ContributorAuthor

I'm happy to take it to servicing if that is the best outcome, but the next servicing date is in January so no fixes would be available until then. I suspect it will work better for you to find a workaround

Are you able to use the diagnostics client instead of the environment variables? https://learn.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

There is already a workaround in that library and you wouldn't have to wait on any servicing fixes, the client and runtime support attaching to your own process and attaching at startup, so it should work for most scenarios

davmason added a commit to davmason/runtime that referenced this pull request Nov 2, 2022
carlossanlop pushed a commit that referenced this pull request Nov 7, 2022
…Pipe sessions (#75248) (#77811)
* Update provider_compute_event_enable_mask so EventSouces with no keywords show up by default (#75248)
Update ep-provider.c
* Update ep-provider.c
* Update src/native/eventpipe/ep-provider.c
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set providers using EventPipeConfig

5 participants

@davmason@noahfalk@mikelle-rogers@adamsitnik@lateralusX