diagnostics_channel: add opt-in subscriber suppression via suppressedBy and suppressed()

[DivyanshuX9]

Summary

Adds opt-in subscriber suppression to diagnostics_channel so APM
agents can prevent recursive instrumentation when their own internal
code calls into instrumented libraries.

What Changed

lib/diagnostics_channel.js

• Added suppressed(key, fn, thisArg, ...args) - runs fn with
  the given key active in the current async context
• subscribe() and bindStore() accept { subscriberId } option
• Separate lazy storage for bypassed subscribers/stores - normal
  path has zero overhead, ALS is only hit when bypass subscribers exist
• Lazy AsyncLocalStorage initialization via getSuppressionsStorage()
  • required because diagnostics_channel loads during bootstrap before
    async_hooks is available
• Unified validateBypassKey() helper for all key validation

test/parallel/test-diagnostics-channel-suppression.js

• 10 test scenarios covering sync, async, Promise, timer,
  store, nested, and type validation cases

Why

APM agents currently implement suppression themselves using a
custom ALS noop marker - every vendor reimplements the same
pattern differently. Moving it into core:

• Eliminates per-handler overhead on the hot path
• Gives vendors a standard API
• Correctly propagates across Promise/timer boundaries via ALS

Backward Compatibility

• Fully opt-in : existing code unaffected
• Normal subscribers pay zero extra cost
• ALS only initialized and queried when bypass subscribers exist

Naming

Currently using suppressed/subscriberId - pending team vote
on final naming (bypass, suppress, silence, or ignore).
Will rename in a single commit once consensus is reached.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[DivyanshuX9]

@BridgeAR @bengl @Qard , I am opening this as a implementation
of #63623. Test file is included. Happy to adjust the API shape
or implementation based on your feedback before CI runs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.35%. Comparing base (79def6d) to head (e630f72).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63651      +/-   ##
==========================================
+ Coverage   90.32%   90.35%   +0.02%     
==========================================
  Files         732      732              
  Lines      236435   236573     +138     
  Branches    44527    44556      +29     
==========================================
+ Hits       213563   213749     +186     
+ Misses      14589    14539      -50     
- Partials     8283     8285       +2     

Files with missing lines	Coverage Δ
lib/diagnostics_channel.js	98.31% <100.00%> (+0.17%)	⬆️

... and 50 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Qard]

Tests are missing a lot of necessary common.mustCall(fn) wrappers.

Also, it seems like this was just vibe-coded without reviewing the output before submitting the PR. Please ensure it is in a good state and that the test suite and lint passes before submitting.

[DivyanshuX9]

@Qard , thanks for the detailed review. So far the checklist of what i have fixed in the latest push:

[] Removed the impossible ALS try/catch fallback
[] Removed node_errors.cc changes as you suggested
[] Replaced all IIFE wrappers with plain blocks in tests
[] Added common.mustCall() / common.mustNotCall() to all handlers
[] Ran lint and tests locally , both passing

Ready for re-review and any changes if you want, when you have time please review it.

[Qard]

The node_errors.cc change still seems to be present.

[DivyanshuX9]

Is this naming you want @BridgeAR ?

Old name	New name
suppressed (function)	bypass
subscriberId (option)	bypassId
suppressionStorage	bypassStorage
getSuppressionsStorage	getBypassStorage

have a quick look so i would start changes

[DivyanshuX9]

Also for the documentation i think we should open new issue/pr , is this fine @BridgeAR ?

[BridgeAR]

@DivyanshuX9 yes, I like that naming more, while I think it would be agreed upon by @nodejs/diagnostics as a whole.

What about a short survey:

❤️ bypass
👍 suppress
🚀 silence
🎉 ignore

In addition: we did not yet agree on the API. @bengl asked for #63623 (comment)

I do not think this would work as suggested, while the current callback API would (it is meant as drop in replacement for storage.run({ noop: true }, fn): suppressed(kMyTracer, fn). We could even introduce a using _ = suppressing(kMyTracer) as alternative to the callback (I would just not do that right now).

[DivyanshuX9]

@BridgeAR , i'll just voted ❤️ for bypass, agrees with your reasoning.

On the API shape , I implemented the callback style bypass(kMyTracer, fn) as a drop-in for storage.run({ noop: true }, fn) which I think is the cleaner approach.

Happy to wait for @bengl and @nodejs/diagnostics to weigh in before making further changes to the API shape.

Should I hold off on the rename until the vote settles, or go ahead with bypass now since that seems to be the leading preference?

Happy to rename again whenever the vote settles although I've gotten good at find-and-replace at this point 😄

(Note: currently still named suppressed/subscriberId in the code will rename to whatever the team settles on from the vote)

[DivyanshuX9]

@BridgeAR ,I fixed two issues in the latest push:

1. Test 8 was using ch.publish() which doesn't enter stores , only ch.runStores() does. Updated the test accordingly.

2. Fixed withSuppressionsContext to use ReflectApply instead of fn.bind() for correct thisArg/args handling.

CI is running. Once the naming vote settles I'll do the full rename in a single commit.

[rochdev]

How is this meant to be used? I've been following the issue and this PR but I still don't understand how this would be used in the real world 😅 Can proper examples be added somewhere?

[DivyanshuX9]

@rochdev ok that's a good question, feature is not for application developers, it's for observability vendors (Datadog, OpenTelemetry, New Relic, Elastic APM, Sentry, etc.).
I'll try in layman terms to explain
An applicable example in the real world would be an APM/tracing tool like Datadog or OpenTelemetry.
When making an HTTP request with your application, as the tracer sees the request and collects telemetry information about the request.
Next the tracer will try to send that telemetry to its backend, which will usually use an HTTP request to do so.
The problem that exists is that the tracer can also see that second HTTP request and collect telemetry about its own internal operation, thus in real life APMs have to add logic to ignore their internal operations, such as exporting telemetry, logging, etc...
INSHORT:
This PR attempts to create a unified way for any tooling to say [to themselves] "this operation is internal, please do not record this again", so that different APMs are not forced to develop their respective workarounds to this same problem.
To put it simply, this is designed mainly for the observability tooling space, to ensure that any tooling does not unintentionally instrument their own telemetry exports, logs, etc... or their internal operations.

I can add a similar example to the PR description(hopefully if i get permission to edit docs ) if this clarifies the use case.

[rochdev]

I can add a similar example to the PR description(hopefully if i get permission to edit docs ) if this clarifies the use case.

That would help a lot! I understand the underlying need (I work at an APM vendor after all) but I don't understand how this solves the issue in the correct way. It seems to me that it introduces coupling between the publisher and subscriber and goes in the opposite direction we're trying to go (upstreaming instrumentation in the libraries themselves so everyone can benefit without every vendor having their own instrumentation). I may be wrong though as without an example I'm not 100% sure how this is meant to be used, especially when the publisher and subscribers are in separate projects.

[bengl]

This needs to handle TracingChannel casses where suppression is triggered on the subscriber side, like I mentioned in the original issue.

Also, let's get rid of the word "suppression" and replace it with "bypass" everywhere.

[DivyanshuX9]

@rochdev great point, let me clarify with a concrete example.

The publisher does not need to know anything about suppression.
Only the subscriber opts in:

// Datadog's HTTP tracer subscribes and says "skip me when I'm 
// doing my own internal work"
const kDatadog = Symbol('datadog');
channel('http.request').subscribe(handler, { subscriberId: kDatadog });

// When Datadog exports traces (internal HTTP request), it wraps 
// that call in suppressed() so its own handler is skipped
suppressed(kDatadog, () => {
  exportTracesToBackend(); // makes HTTP request internally
});
// handler is NOT called for this internal request
// handler IS called for all application HTTP requests

The publisher (http.request channel) is completely unaware of suppression ,so it just calls publish() as normal. Only the subscriber (Datadog) opts into being skipped during its own internal operations. No coupling between publisher and subscriber.

Happy to add this example to the PR description, if it feels more helpful.

[DivyanshuX9]

@bengl ,ok thanks for the review. A few questions before I make changes:

1. On TracingChannel -> can you point to the specific case you have in mind? The current implementation forwards bypassId through TracingChannel.subscribe() to the underlying channels. I want to make sure I understand whether the concern is about:

   a) The subscribe() path (bypassId not reaching underlying channels)
   b) The tracePromise/traceCallback async continuation path
   (suppression context not propagating across async boundaries)
   c) Something else entirely

2. On the original enterWith API -> I implemented the callback style as BridgeAR suggested. Now that you're reviewing the PR directly, do you want me to switch to the enterWith style (ch.suppressFor(key) / ch.unsuppressFor(key))? I want to make sure we have consensus before I change the API shape again.

On the rename to bypass -> doing that now.