-
Notifications
You must be signed in to change notification settings - Fork 772
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
Metrics are obsolete #1611
Metrics are obsolete #1611
Conversation
@@ -5,7 +5,7 @@ | |||
<!-- | |||
TODO: Disable this exception, and actually do document all public API. | |||
--> | |||
<NoWarn>$(NoWarn),1591</NoWarn> | |||
<NoWarn>$(NoWarn),1591,CS0618</NoWarn> |
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.
Will open a new issue to move CS0618 nowarn to individual metric related files, not for whole project.
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.
@Austin-Tan Can you work on this? (a separate PR)
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.
Sure I'll get started right now. Would you recommend just Ctrl+F'ing the project for references to components in src/OpenTelemetry/Metrics, and appending #pragma warning disable cs0618
to the top?
And double checking my understanding - why don't we want compiler warnings? Aren't we intending to make it clear that Metrics are experimental?
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.
Metrics are marked Obsolete so customers should be alerted. But there are pieces in this repo itself which depends on the metric, so they should be excluded from this warning.
(current its a project level warning exclusion, which will prevent us from catching any real issues, outside of metrics).
Okay to merge once we get 2 approvals. |
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
Fixes #1370.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes