Streamline EvenSources

[pentp]

• Simplify level/keyword based IsEnabled checks.
• Delete some dead code.
• Streamline TplEventSource use.

[Copilot]

Pull request overview

This PR streamlines event-source–based tracing in the TPL and EventSource infrastructure by simplifying IsEnabled paths, removing unused plumbing, and tightening how the default TaskScheduler and ETW/EventPipe activity IDs are handled.

Changes:

• Simplifies many TplEventSource call sites and related Task/await continuation logging by relying on a cheaper EventSource.IsEnabled(EventLevel, EventKeywords) check and centralizing some repeated logic.
• Adjusts TaskScheduler/ThreadPoolTaskScheduler initialization so the default scheduler’s ID is explicitly set and used via a new DefaultId constant, reducing redundant ID creation calls while preserving existing ID semantics.
• Refactors EventSource’s enablement state (level/keywords) and removes unused ActivityIdControl overrides in EventProvider/EventPipeEventProvider, tightening the level/keyword quick checks while keeping per-session filtering behavior consistent.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
System.Threading.Tasks/TplEventSource.cs	Simplifies IsEnabled checks to use the level/keyword overload directly and replaces a manual WriteEventCore payload for synchronous-work stop with a typed WriteEvent call.
System.Threading.Tasks/ThreadPoolTaskScheduler.cs	Removes the constructor that eagerly forced default scheduler ID creation, deferring to the new DefaultId-based initialization in TaskScheduler.
System.Threading.Tasks/TaskScheduler.cs	Makes the default scheduler strongly typed as ThreadPoolTaskScheduler, introduces DefaultId = 1, and initializes the scheduler ID counter starting from this default ID.
System.Threading.Tasks/TaskContinuation.cs	Updates ETW logging for await continuations to use task.ExecutingTaskScheduler?.Id ?? TaskScheduler.DefaultId, aligning with the new default-ID convention.
System.Threading.Tasks/Task.cs	Refactors completion causality logging to compute a single AsyncCausalityStatus, streamlines task-start/complete and wait ETW logging to use nullable TplEventSource and TaskScheduler.DefaultId, and inlines the completion-notification logging helper.
System.Runtime.CompilerServices/TaskAwaiter.cs	Switches wait ETW logging to use TaskScheduler.DefaultId instead of forcing access to TaskScheduler.Default.Id.
System.Diagnostics.Tracing/EventSource.cs	Reworks IsEnabled(EventLevel, EventKeywords) to be a fast check over m_level/m_matchAnyKeyword, refactors IsEnabledCommon to use new sentinels (EventLevel -1 and EventKeywords.All), ensures level state is reset on dispose/disable, and slightly cleans up activity-ID tracking in WriteEventWithRelatedActivityIdCore.
System.Diagnostics.Tracing/EventProvider.cs	Removes now-unused virtual ActivityIdControl hooks from EventProviderImpl and its ETW implementation, since activity-id control is handled directly via interop or EventPipeEventProvider.
System.Diagnostics.Tracing/EventPipeEventProvider.cs	Drops the override of ActivityIdControl in favor of the existing static EventActivityIdControl helper used by EventSource.

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @dotnet/area-system-diagnostics-tracing
See info in area-owners.md if you want to be subscribed.

[stephentoub]

Relying on the implicit relationship between Id and m_taskId makes this harder to read. Why not just have a taskId local?

[stephentoub]

I'm not a fan of these gotos. The work being jumped to is too far away to reconcile mentally.

[stephentoub]

What do these changes help? This code is run exactly once.

[noahfalk]

I wouldn't try to inline a custom implementation here unless there is a clear need to do so like a perf benchmark for a meaningful scenario + a good set of unit tests validating the truth table. We've had several bugs in the past from mismatched implementations of event enable logic.

[noahfalk]

I get the desire to avoid the special case 0 behavior on EventLevel and EventKeywords - its an annoying convention that creates an edge case for what could have been a simple comparison. However I don't think we are making things simpler globally when we change the convention in some places but keep it in others. This convention isn't just in EventSource, it also shows up in EventPipe and ETW going back 20+ years. Carving out a little area where the convention doesn't hold just invites a different kind of confusion.
I think we are better off if we accept the warts on the existing convention rather than trying to create a 2nd convention.

[noahfalk]

This logic appears different than the previous logic assuming that m_level = 0 previously is the same as m_level = MaxValue after the change.

If we start two sessions, one with commandArgs.level=1 and the next with commandArgs.level=0
Before: m_level=1 at the end
After: m_level = MaxValue at the end

The old behavior looks like a bug so I'd be happy to see it fixed, but I wouldn't want to do that in a PR that describes itself as being a refactoring. I'd also suggest fixing it staying within the existing convention so the new behavior would be m_level=0 at the end, not MaxValue.

[stephentoub]

🤖 Copilot Code Review — PR #123885

Holistic Assessment

Motivation: The PR aims to streamline EventSource/TplEventSource usage by simplifying IsEnabled checks, removing dead code (ActivityIdControl), and reducing redundant operations like TaskScheduler.Default.Id access. The motivation is reasonable—reducing overhead in hot paths.

Approach: The changes introduce a new sentinel convention (m_level = -1 for disabled, EventKeywords.All for "all keywords") that differs from the longstanding ETW/EventPipe convention where 0 means "match all." While the code may be functionally correct, the approach creates inconsistency and maintenance risk.

Summary: ⚠️ Needs Human Review. The PR contains behavioral changes beyond what's described as "refactoring" (enable logic with level=0), relies on a new sentinel convention that conflicts with established ETW patterns, and has raised valid concerns from maintainers. The inline IsEnabled optimization may be correct but lacks test coverage for the new truth table.

---

Detailed Findings

⚠️ Semantic Change in Enable Logic — Level=0 behavior changed

Location: EventSource.cs DoCommand (~lines 2752-2755)

Old behavior:

if (commandArgs.level > m_level)
    m_level = commandArgs.level;

New behavior:

EventLevel commandLevel = commandArgs.level == 0 ? (EventLevel)int.MaxValue : commandArgs.level;
m_level = commandLevel > m_level ? commandLevel : m_level;

If two sessions enable with level=1 then level=0:

• Before: m_level = 1 (0 doesn't update because the comparison uses raw values)
• After: m_level = int.MaxValue (0 is converted to MaxValue, which > 1)

This is a behavioral change, not a refactoring. As noahfalk noted, if this is a bug fix, it should be explicit and separate. This change alters how multiple listeners with different levels interact.

---

⚠️ Inconsistent Sentinel Convention — Mixing -1/MaxValue/0

Location: EventSource.cs

The PR introduces conflicting conventions:

• m_level = (EventLevel)(-1) as the "disabled" sentinel
• (EventLevel)int.MaxValue as the "all levels" sentinel in conversion
• Legacy: 0 means "match all" in ETW/EventPipe (still used externally)

This creates cognitive overhead and bug risk. The established convention (level=0 means all, keywords=0 means all) has 20+ years of history in ETW and is used throughout the tracing stack. Creating a private "translation layer" where different values mean the same thing invites future bugs when code is modified or extended.

Recommendation: Either stay within the existing convention (accepting its warts) or propose a comprehensive migration of the entire stack, not a partial change.

---

⚠️ IsEnabled(EventLevel, EventKeywords) Inlining — Needs Justification

Location: EventSource.cs lines 318-327

The new inline implementation relies on the -1 sentinel to implicitly handle the disabled case:

public bool IsEnabled(EventLevel level, EventKeywords keywords)
{
    if (m_level < level)  // -1 < any_valid_level, so returns false when disabled
        return false;

    if (keywords != 0 && (keywords & m_matchAnyKeyword) == 0)
        return false;

    return true;
}

While this appears functionally equivalent due to the sentinel, it:

1. Drops the explicit m_eventSourceEnabled check, relying on implicit sentinel behavior
2. Drops channel handling (different from 3-arg overload)
3. Has no test coverage for the new truth table

As noahfalk noted, EventSource enable logic has had bugs in the past from mismatched implementations. Without benchmarks showing this hot path needs optimization and comprehensive tests validating the truth table, this change adds risk for unclear benefit.

---

⚠️ Goto Statements in RunContinuations — Reduced Readability

Location: Task.cs lines 3473, 3478, 3483, 3488, 3592

The switch cases now use goto LogFinishCompletionNotification where the label is ~100 lines away. As stephentoub noted, the work being jumped to is too far away to reconcile mentally.

Recommendation: Revert to the helper method pattern LogFinishCompletionNotification() or use a local function if inlining is truly needed.

---

💡 Implicit Id/m_taskId Relationship — Minor Readability Issue

Location: Task.cs line 2311

_ = Id;
if (log.TasksSetActivityIds)
    EventSource.SetCurrentThreadActivityId(TplEventSource.CreateGuidForTaskID(m_taskId), out savedActivityID);

The _ = Id forces ID creation, then m_taskId is used directly. This relies on implicit knowledge that Id populates m_taskId. As stephentoub noted, a local variable would be clearer:

int taskId = Id;
// use taskId consistently

---

💡 ThreadPoolTaskScheduler Changes — Unclear Value

Location: ThreadPoolTaskScheduler.cs, TaskScheduler.cs

Removing the constructor and using object initializer { m_taskSchedulerId = DefaultId } provides no measurable benefit. The code runs exactly once at startup. This is churn without clear improvement.

---

✅ Dead Code Removal — ActivityIdControl

Location: EventProvider.cs, EventPipeEventProvider.cs

Removing the unused ActivityIdControl virtual methods is appropriate cleanup. These were never called.

---

✅ TaskScheduler.DefaultId Constant

Location: TaskScheduler.cs, call sites

Using TaskScheduler.DefaultId instead of TaskScheduler.Default.Id avoids a property access in logging paths. This is a minor but valid optimization.

---

Recommendations

1. Separate concerns: Split behavioral changes (level=0 handling) from refactoring (code cleanup) into separate PRs with appropriate testing.

2. Reconsider sentinel convention change: The -1/MaxValue sentinel approach conflicts with established ETW conventions. Consider staying within the existing 0-means-all convention.

3. Add tests: If the IsEnabled inlining is kept, add comprehensive tests covering:

   • Disabled EventSource with various levels/keywords
   • level=0 and keywords=0 edge cases
   • Multiple enable/disable cycles with different levels

4. Revert goto pattern: Use helper method or local function instead of long-distance gotos.

5. Provide benchmarks: If this is a performance optimization, provide BenchmarkDotNet numbers showing the improvement.

---

Review generated by Copilot. Models contributing: Claude Opus 4.5.

[steveisok]

@pentp given the amount of feedback, I converted the PR to draft. Please feel free to click 'ready to review' when you want us to take a look again.

[dotnet-policy-service]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.