Skip to content

Conversation

hannahhaering
Copy link
Contributor

@hannahhaering hannahhaering commented Oct 3, 2025

Implements #4155

Changes

  • Implemented OTEL_SDK_DISABLED according to the OpenTelemetry specification
  • Applied support across TracerProvider, MeterProvider, and LoggerProvider

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@github-actions github-actions bot added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Oct 3, 2025
Copy link

codecov bot commented Oct 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.32%. Comparing base (0ea6c23) to head (d35c752).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6568      +/-   ##
==========================================
- Coverage   86.67%   86.32%   -0.36%     
==========================================
  Files         258      258              
  Lines       11895    11910      +15     
==========================================
- Hits        10310    10281      -29     
- Misses       1585     1629      +44     
Flag Coverage Δ
unittests-Project-Experimental 86.06% <100.00%> (-0.53%) ⬇️
unittests-Project-Stable 86.11% <100.00%> (-0.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...elemetry/Logs/Builder/LoggerProviderBuilderBase.cs 97.36% <100.00%> (+0.39%) ⬆️
...emetry/Metrics/Builder/MeterProviderBuilderBase.cs 97.56% <100.00%> (+0.33%) ⬆️
...lemetry/Trace/Builder/TracerProviderBuilderBase.cs 98.24% <100.00%> (+0.16%) ⬆️

... and 5 files with indirect coverage changes

@hannahhaering hannahhaering marked this pull request as ready for review October 3, 2025 23:55
@hannahhaering hannahhaering requested a review from a team as a code owner October 3, 2025 23:55
Comment on lines 11 to 20
public LoggerProviderBuilderBaseTests()
{
Environment.SetEnvironmentVariable(SdkConfigDefinitions.SdkDisableEnvVarName, null);
}

public void Dispose()
{
Environment.SetEnvironmentVariable(SdkConfigDefinitions.SdkDisableEnvVarName, null);
GC.SuppressFinalize(this);
}
Copy link
Member

Choose a reason for hiding this comment

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

A better way to do this, IMHO, would be to:

  1. Not set the value in the constructor
  2. Let the test set the value it's interested in
  3. Set it back to the original value when being disposed

You could use a similar pattern to this to implement it in a cross-cutting way, then the usage would be something like:

using (new EnvironmentVariableScope("OTEL_SDK_DISABLED", value))
{
    var builder = new LoggerProviderBuilderBase();

    using var provider = builder.Build();

    Assert.IsType(expected, provider);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I've done that.

Is there any way I can retry the workflow without a commit? Sometimes the build fails even though I have no errors (like right now).

Copy link
Member

Choose a reason for hiding this comment

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

Not without write access to the repo, no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants