-
Notifications
You must be signed in to change notification settings - Fork 784
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
Rename AspNetCoreInstrumentationOptions #5108
Rename AspNetCoreInstrumentationOptions #5108
Conversation
…ub.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/rename-instrumentationOptions
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5108 +/- ##
=======================================
Coverage 83.11% 83.11%
=======================================
Files 296 296
Lines 12314 12314
=======================================
Hits 10235 10235
Misses 2079 2079
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
Q: Should these types of classes use plural naming?
vs
|
Not a strong preference but I'd opt for the singular form since the singularity pertains to instrumentation (Unless there's a specific naming convention guideline you're referring to). Also, how does this differ from other names where we use singular convention e.g. |
I was just chatting with @utpilla about the names. I doubt there is much consistency around 😄 I was just thinking reading this PR that these options classes are configuring at the signal level so they should probably use the signal names. I feel like the signal names are "Tracing", "Metrics", and "Logging" but probably also inconsistency there between "Traces", "Metrics", and "Logs" in the spec. I don't feel passionately about it just thought I would bring it up. We can discuss on the SIG a bit. Fine to go with these names for now. Just looking at the folder structure in java https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk it would seem we are not alone in our inconsistency. Mixed bag "logs", "metrics", and "trace" folders there 🤣 |
Fixes #
Design discussion issue #
Changes
Renaming the class to make the intent more clear. It keeps the option to introduce metric options separately in future (if needed).
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes