Skip to content

Conversation

@Kielek
Copy link
Member

@Kielek Kielek commented Oct 6, 2025

Fixes #3166

Changes

[AspNet.Telemetry] Fix end of time by utilizing implementation from https://github.com/dotnet/runtime/blob/75662173e3918f2176b74e467dc8e41d4f01d4d4/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.DateTime.netfx.cs

Alternative approach - use some reflection/delegates to call internally visible methods.

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)

@Kielek Kielek marked this pull request as ready for review October 6, 2025 09:37
@Kielek Kielek requested a review from a team as a code owner October 6, 2025 09:37
@github-actions github-actions bot added the comp:instrumentation.aspnet.telemetryhttpmodule Things related to OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule label Oct 6, 2025
@codecov
Copy link

codecov bot commented Oct 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.81%. Comparing base (fb89d09) to head (6d3fe55).
⚠️ 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    #3171      +/-   ##
==========================================
- Coverage   69.92%   65.81%   -4.12%     
==========================================
  Files         439      418      -21     
  Lines       16939    15853    -1086     
==========================================
- Hits        11844    10433    -1411     
- Misses       5095     5420     +325     
Flag Coverage Δ
unittests-Instrumentation.AspNet 76.24% <100.00%> (+1.24%) ⬆️
unittests-Instrumentation.Cassandra ?

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

Files with missing lines Coverage Δ
...pNet.TelemetryHttpModule/ActivityDateTimeHelper.cs 100.00% <100.00%> (ø)
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs 85.00% <100.00%> (ø)

... and 91 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Kielek
Copy link
Member Author

Kielek commented Oct 6, 2025

@jordankay13, could you please check changes from this PR? You should be able to download alpha package from https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/18277611950?pr=3171

or compile it locally.

@jordankay13
Copy link

@jordankay13, could you please check changes from this PR? You should be able to download alpha package from https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/18277611950?pr=3171

or compile it locally.

Thanks for this! I have a bit more work to do to get on the new TelemetryHttpModule contract (we aren't using the AspNet instrumentation so removing the Activity creation causes some minor headaches -- this PR motivates me to get on latest TelemetryHttpModule, so I'll let you know when I get to it).

FWIW - we put this same fix into our app last week and overwrote the span EndTime in the OnRequestStoppedCallback and we noticed a huge improvement, so I have no reason to doubt the code in this PR.

@Kielek
Copy link
Member Author

Kielek commented Oct 6, 2025

Thanks for this! I have a bit more work to do to get on the new TelemetryHttpModule contract (we aren't using the AspNet instrumentation so removing the Activity creation causes some minor headaches -- this PR motivates me to get on latest TelemetryHttpModule, so I'll let you know when I get to it).

Sorry for that, your problems are probably related to #3151. Needed to make both packages stable.

If you do not care about attributes/otel. semantic convention, you can just create your acivity in any way you want.

@Kielek Kielek merged commit 89c29bb into open-telemetry:main Oct 6, 2025
61 checks passed
@Kielek Kielek deleted the aspnet-endofspan branch October 6, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:instrumentation.aspnet.telemetryhttpmodule Things related to OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] TelemetryHttpModule sets an inaccurate EndTime

4 participants