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

Rename AspNetCoreInstrumentationOptions #5108

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Dec 1, 2023

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

  • 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)

@vishweshbankwar vishweshbankwar changed the title Rename AspNetCoreInstrumentationOption Rename AspNetCoreInstrumentationOptions Dec 1, 2023
@vishweshbankwar vishweshbankwar marked this pull request as ready for review December 1, 2023 02:21
@vishweshbankwar vishweshbankwar requested a review from a team as a code owner December 1, 2023 02:21
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Merging #5108 (e17a5b8) into main (94c20c6) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5108   +/-   ##
=======================================
  Coverage   83.11%   83.11%           
=======================================
  Files         296      296           
  Lines       12314    12314           
=======================================
  Hits        10235    10235           
  Misses       2079     2079           
Flag Coverage Δ
unittests 83.11% <100.00%> (ø)

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

Files Coverage Δ
...spNetCore/AspNetCoreTraceInstrumentationOptions.cs 100.00% <ø> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 75.18% <ø> (ø)
...tion.AspNetCore/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>
@CodeBlanch
Copy link
Member

Q: Should these types of classes use plural naming?

AspNetCoreTracingInstrumentationOptions
AspNetCoreMetricsInstrumentationOptions
AspNetCoreLoggingInstrumentationOptions

vs

AspNetCoreTraceInstrumentationOptions
AspNetCoreMetricInstrumentationOptions
AspNetCoreLogInstrumentationOptions

@vishweshbankwar
Copy link
Member Author

vishweshbankwar commented Dec 1, 2023

Q: Should these types of classes use plural naming?

AspNetCoreTracingInstrumentationOptions
AspNetCoreMetricsInstrumentationOptions
AspNetCoreLoggingInstrumentationOptions

vs

AspNetCoreTraceInstrumentationOptions
AspNetCoreMetricInstrumentationOptions
AspNetCoreLogInstrumentationOptions

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. OtlpMetricExporter and not OtlpMetricsExporter?

@utpilla utpilla merged commit c48c014 into open-telemetry:main Dec 1, 2023
78 checks passed
@CodeBlanch
Copy link
Member

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 🤣

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.

None yet

5 participants