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

[Hosting] Throw startup exceptions #4006

Merged
merged 6 commits into from
Dec 14, 2022

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Dec 13, 2022

Relates to #3753

Changes

  • The IHostedService used by OpenTelemetry.Extensions.Hosting will now throw if it blows up starting TracerProvider or MeterProvider.

Details

  • If you do this today using the "Sdk.Create" style you will get a NotSupportedException:

         using var provider = Sdk.CreateTracerProviderBuilder()
             .ConfigureBuilder((sp, sdkBuilder) =>
             {
                 // Note: This throws because services cannot be
                 // registered after IServiceProvider has been
                 // created.
                 sdkBuilder.SetSampler<MySampler>();
             })
             .Build();
  • If you do this today using the hosting style an internal log is written but otherwise nothing happens:

             services.AddOpenTelemetry()
                 .WithTracing(builder =>
                 {
                     builder.ConfigureBuilder((sp, sdkBuilder) =>
                     {
                         // Note: This throws because services cannot be
                         // registered after IServiceProvider has been
                         // created.
                         sdkBuilder.SetSampler<MySampler>();
                     });
                 })
                 .StartWithHost();

The goal of the change is to make the behavior consistent and notify users when they have made a mistake.

The above example is a dev issue but other kinds of problems will also now blow up the process. Bad configuration, invalid permissions, port issues, etc. Discussing with @alanwest we felt it was better to surface these types of issues immediately and prevent the process from starting.

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes

@CodeBlanch CodeBlanch requested a review from a team December 13, 2022 23:58
[NonEvent]
public void FailedInitialize(Exception ex)
[Event(1, Message = "OpenTelemetry TraverProvider and/or MeterProvider were not found in application services. SDK will remain disabled.", Level = EventLevel.Warning)]
public void SdkNotRegistered()
Copy link
Contributor

Choose a reason for hiding this comment

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

Have dedicated events for TracerProvider and MeterProvider instead of using the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #4006 (6c1b075) into main (733a868) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4006      +/-   ##
==========================================
+ Coverage   85.46%   85.48%   +0.01%     
==========================================
  Files         289      289              
  Lines       11237    11229       -8     
==========================================
- Hits         9604     9599       -5     
+ Misses       1633     1630       -3     
Impacted Files Coverage Δ
...ing/Implementation/HostingExtensionsEventSource.cs 100.00% <100.00%> (+54.54%) ⬆️
...s.Hosting/Implementation/TelemetryHostedService.cs 100.00% <100.00%> (+26.66%) ⬆️
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (-15.00%) ⬇️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs 72.72% <0.00%> (-13.64%) ⬇️
...tpListener/Internal/PrometheusCollectionManager.cs 73.62% <0.00%> (-2.20%) ⬇️
src/OpenTelemetry/BatchExportProcessor.cs 82.24% <0.00%> (-1.87%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️
...lementation/SqlClientInstrumentationEventSource.cs 75.00% <0.00%> (+4.16%) ⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

@CodeBlanch CodeBlanch merged commit a44a58b into open-telemetry:main Dec 14, 2022
@CodeBlanch CodeBlanch deleted the hosting-startup-exceptions branch December 14, 2022 21:19
}
// The sole purpose of this HostedService is to ensure all
// instrumentations, exporters, etc., are created and started.
Initialize(this.serviceProvider);
Copy link
Member

Choose a reason for hiding this comment

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

nit: add a comment saying intentionally not catching ex, to let it bubble up.

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.

4 participants