Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Missing Endpoint #5135

Merged
merged 8 commits into from
Jan 27, 2024
Merged

Fixed Missing Endpoint #5135

merged 8 commits into from
Jan 27, 2024

Conversation

cliedeman
Copy link
Contributor

@cliedeman cliedeman commented Dec 6, 2023

When an exception if processed by the ExceptionHandler middleware the original endpoint is deleted

Fixes #5134

Changes

Source endpoint from exception handler feature

Merge requirement checklist

  • CONTRIBUTING guidelines followed (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)

@cliedeman cliedeman requested a review from a team December 6, 2023 11:10
@cliedeman cliedeman marked this pull request as draft December 7, 2023 08:42
@cliedeman cliedeman marked this pull request as ready for review December 7, 2023 11:42
@cliedeman
Copy link
Contributor Author

I have finished the code changes but I am struggling with the tests.

The new test case currently passest on dotnet 7 but fails on 8 with the metrics route being empty.

@vishweshbankwar
Copy link
Member

I have finished the code changes but I am struggling with the tests.

The new test case currently passest on dotnet 7 but fails on 8 with the metrics route being empty.

@cliedeman, Thank you for reporting the issue and proposing a solution. It looks good, but given its impact on metrics instrumentation, it would be good to get confirmation from @JamesNK before proceeding. Metrics instrumentation is done natively starting .NET 8.0 and it is following the same logic as we do currently. Anything we update here, we would like to keep the changes in sync so that the experience is consistent across different .NET versions.

@cliedeman
Copy link
Contributor Author

Thanks @vishweshbankwar that explains the change in behaviour.

@JamesNK
Copy link
Contributor

JamesNK commented Dec 8, 2023

Thanks for the heads up. I've created an aspnetcore issue to look into this - dotnet/aspnetcore#52648

Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Dec 15, 2023
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Dec 23, 2023
@cliedeman
Copy link
Contributor Author

Blocked on #52648 being back ported

@alanwest
Copy link
Member

alanwest commented Jan 2, 2024

Blocked on #52648 being back ported

Noted here dotnet/aspnetcore#52648 (comment), this fix will be available in 8.0.2. It only addresses things for metrics.

I'm reopening your PR as your fix to this instrumentation is still valuable because it addresses both traces and metrics as well as benefitting .NET 6 and 7 users.

@alanwest alanwest reopened this Jan 2, 2024
@alanwest alanwest removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jan 3, 2024
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (6250307) 83.38% compared to head (e9a836b) 83.07%.
Report is 34 commits behind head on main.

❗ Current head e9a836b differs from pull request most recent head e340c76. Consider uploading reports for the commit e340c76 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5135      +/-   ##
==========================================
- Coverage   83.38%   83.07%   -0.32%     
==========================================
  Files         297      272      -25     
  Lines       12531    11985     -546     
==========================================
- Hits        10449     9956     -493     
+ Misses       2082     2029      -53     
Flag Coverage Δ
unittests ?
unittests-Instrumentation-Experimental 25.07% <90.00%> (?)
unittests-Instrumentation-Stable 25.07% <90.00%> (?)
unittests-Solution-Experimental 83.04% <44.82%> (?)
unittests-Solution-Stable 82.99% <44.82%> (?)

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

Files Coverage Δ
...etryProtocol/Implementation/ExperimentalOptions.cs 100.00% <ø> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs 100.00% <100.00%> (ø)
...rotocol/Implementation/OtlpLogRecordTransformer.cs 93.45% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 89.79% <100.00%> (+0.21%) ⬆️
...AspNetCore/Implementation/HttpInMetricsListener.cs 89.74% <100.00%> (+0.26%) ⬆️
...plementation/HttpWebRequestActivitySource.netfx.cs 80.77% <100.00%> (ø)
...emetry/Metrics/Exemplar/SimpleExemplarReservoir.cs 80.43% <100.00%> (ø)
...mentation/Retry/OtlpExporterTransmissionHandler.cs 0.00% <0.00%> (ø)

... and 34 files with indirect coverage changes

@cliedeman
Copy link
Contributor Author

Thanks @alanwest will address the feedback soon

@cliedeman
Copy link
Contributor Author

@alanwest I have address the feedback. How do you suggest we make the tests pass for 8 until the fix is released?

Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jan 24, 2024
@vishweshbankwar
Copy link
Member

@cliedeman Could you address the comments from @alanwest? We will move forward with the merge then. Thanks!

cliedeman and others added 4 commits January 26, 2024 23:34
When an exception if processed by the ExceptionHandler middleware the original endpoint is deleted
…/TestApplication/TestApplicationFactory.cs

Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com>
@cliedeman
Copy link
Contributor Author

@vishweshbankwar feedback addresed

@alanwest alanwest removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jan 27, 2024
Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

Thanks, @cliedeman!

@alanwest alanwest merged commit 26a2d4b into open-telemetry:main Jan 27, 2024
44 checks passed
alanwest added a commit to alanwest/opentelemetry-dotnet that referenced this pull request Jan 27, 2024
alanwest added a commit that referenced this pull request Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Endpoint when Exception is processed by Exception Handler
4 participants