-
Notifications
You must be signed in to change notification settings - Fork 292
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
[aspnetcore] Restore metrics instrumentation in netstandard builds #2403
Conversation
…pping su…" This reverts commit 3964894.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2403 +/- ##
==========================================
- Coverage 73.91% 73.82% -0.09%
==========================================
Files 267 23 -244
Lines 9615 661 -8954
==========================================
- Hits 7107 488 -6619
+ Misses 2508 173 -2335
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a decision to fully drop .NET6/.NET7 support (without making. the major upgrade here: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2138/files#diff-9fc8151d477e5c83d4231a1da766f08e0f087e880e42e19695175598e4069b4a). If we speaking about lack of technical approval, I assumed that @CodeBlanch is fine with changes, just after fixing the feedback. #2360 (comment). Maybe I was wrong, if so, I am sorry for this. |
Our stance on .NET Standard was discussed at length a couple years ago. While I think your position is a valid one, it is not the one we settled on. Until we decide to change that, and discuss how, we need to stick with our prior decisions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
#2360 needs further discussion. It removes metric functionality from a supported target, so it is a breaking change that warrants a major version bump. That said, I don't see any reason to have removed this functionality at all.
This change needs to be reverted. Also, it appears this was released as 1.10.0. A version 1.10.1 will need to be released to fix this breaking change.
Lastly, I noted that #2360 was merged without the approval of an approver or maintainer.
@Kielek @CodeBlanch