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

Remove options, stick with builder #1875

Merged
merged 5 commits into from
Mar 8, 2021
Merged

Remove options, stick with builder #1875

merged 5 commits into from
Mar 8, 2021

Conversation

reyang
Copy link
Member

@reyang reyang commented Mar 7, 2021

Related to #1858 and #1833.

Changes

I would propose that we only use builder as the configuration, instead of having both builder and options pattern.
This could also help us to have a consistent model when we convert raw metrics to logs, activities to sampled metrics, activity events to logs, and logs to activity events - as discussed here.

Couple benefits:

  • Smaller API surface
  • Avoid the confusion of having two ways of doing things
  • Builder pattern allows people enable/disable things, rather than configuration - once set you're done and there is no way for the others to disable it in their code.

For significant contributions please make sure you have completed the following items:

@reyang reyang requested a review from a team March 7, 2021 17:13
@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #1875 (13b0bfd) into main (cb066cb) will decrease coverage by 0.55%.
The diff coverage is 78.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1875      +/-   ##
==========================================
- Coverage   83.77%   83.22%   -0.56%     
==========================================
  Files         187      188       +1     
  Lines        5967     6129     +162     
==========================================
+ Hits         4999     5101     +102     
- Misses        968     1028      +60     
Impacted Files Coverage Δ
src/OpenTelemetry/Sdk.cs 100.00% <ø> (ø)
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 26.82% <20.54%> (-44.60%) ⬇️
src/OpenTelemetry/Logs/LogRecord.cs 82.50% <77.77%> (-5.00%) ⬇️
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 77.55% <88.23%> (+7.09%) ⬆️
src/OpenTelemetry/Trace/ExceptionProcessor.cs 90.90% <90.90%> (ø)
...rc/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs 89.83% <91.66%> (-0.92%) ⬇️
src/OpenTelemetry/Trace/TracerProviderSdk.cs 95.16% <98.64%> (+4.38%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 86.89% <100.00%> (+0.05%) ⬆️
...ry.Instrumentation.AspNet/AspNetInstrumentation.cs 100.00% <100.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs 89.10% <100.00%> (ø)
... and 24 more

@reyang reyang closed this Mar 7, 2021
@reyang reyang reopened this Mar 7, 2021
@ejsmith
Copy link
Contributor

ejsmith commented Mar 8, 2021

I really don't get the motivation for this. This is a pretty standard pattern. Options is the root thing that can just be new'd up and you can configure the options yourself. But a builder wrapper around that to provide a fluent API for setting up your configuration. It's the exact same pattern that appears about 1000 times all over the .NET generic host and ASP.NET and many many other systems.

@alexvaluyskiy
Copy link
Contributor

OpenTelemetry docs says that SDK should not have any breaking changes. At least for one year. I think, this PR changes the way to configure of OpenTelemetry, and break the compatibility.

@cijothomas
Copy link
Member

OpenTelemetry docs says that SDK should not have any breaking changes. At least for one year. I think, this PR changes the way to configure of OpenTelemetry, and break the compatibility.

There is no breaking change. This PR is reverting an option which was introduced last week (and never released, as indicated by changes to .publicapi.unshipped.txt).

@cijothomas cijothomas merged commit 76d9fc0 into main Mar 8, 2021
@cijothomas cijothomas deleted the reyang/refactor branch March 8, 2021 18:21
/// <param name="tracerProviderBuilder">TracerProviderBuilder instance.</param>
/// <param name="enabled">Enabled or not. Default value is <c>true</c>.</param>
/// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns>
public static TracerProviderBuilder SetErrorStatusOnException(this TracerProviderBuilder tracerProviderBuilder, bool enabled = true)
Copy link
Member

Choose a reason for hiding this comment

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

@reyang Just curious, what's the use case you were thinking of for adding with enabled = false?

Copy link
Member Author

Choose a reason for hiding this comment

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

The spec SIG is working on "Convenience" API. I imagine at certain point we might have customers extending the API and provide something like:

Sdk.CreateTracerProviderBuilder()
    .EnableFamilyPack() // which includes SetErrorStatusOnException(true)
    .SetErrorStatusOnException(false)

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.

6 participants