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

[sdk] Make ResourceBuilder.AddDetector factory pattern public (attempt 2) #4261

Merged
merged 8 commits into from
Mar 6, 2023

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Mar 3, 2023

Fixes #4139
Fixes #4228
Relates to #4103
Relates to #4230

Changes

  • Exposes ResourceBuilder.AddDetector factory pattern overload.

Public API Changes

namespace OpenTelemetry.Resources
{
    public class ResourceBuilder
    {
+       public ResourceBuilder AddDetector(Func<IServiceProvider, IResourceDetector> resourceDetectorFactory) {}
    }
}

Details

The original API that was removed/hidden from 1.4:

public ResourceBuilder AddDetector(Func<IServiceProvider?, IResourceDetector> resourceDetectorFactory) {}

The API being added here:

public ResourceBuilder AddDetector(Func<IServiceProvider, IResourceDetector> resourceDetectorFactory) {}

The difference is the nullable behavior of the IServiceProvider parameter.

With the new API, doing this...

new ResourceBuilder().AddDector(sp => sp.GetRequiredService<MyResourceDetector>()).Build()

...will lead to a NotSupportedException because the ResourceBuilder is not associated to either an IServiceCollection or an IServiceProvider.

Using either of these APIs...

builder.SetResourceBuilder(new ResourceBuilder().AddDector(sp => sp.GetRequiredService<MyResourceDetector>()))
builder.ConfigureResourceBuilder(builder => builder.AddDector(sp => sp.GetRequiredService<MyResourceDetector>()))

...everything will work fine across traces, metrics, and logs.

The idea here is the common use cases will work as expected without the complexity of having to deal with the null case.

I really wanted to also add AddDetector<T>() to be consistent with our other APIs but there isn't a good way for that to work in logs currently and I felt it being consistent for all signals was a good thing. Hopefully we can bring that in for ~1.6.

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@CodeBlanch CodeBlanch requested a review from a team March 3, 2023 18:43
@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #4261 (78f7d10) into main (8cd0ef5) will increase coverage by 0.01%.
The diff coverage is 87.50%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4261      +/-   ##
==========================================
+ Coverage   83.15%   83.17%   +0.01%     
==========================================
  Files         296      296              
  Lines       11775    11789      +14     
==========================================
+ Hits         9792     9805      +13     
- Misses       1983     1984       +1     
Impacted Files Coverage Δ
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 90.90% <50.00%> (-0.85%) ⬇️
src/OpenTelemetry/ProviderExtensions.cs 95.00% <50.00%> (-5.00%) ⬇️
src/OpenTelemetry/Resources/ResourceBuilder.cs 91.52% <100.00%> (+1.72%) ⬆️
...enTelemetry/Resources/ResourceBuilderExtensions.cs 92.85% <100.00%> (ø)
...tpListener/Internal/PrometheusCollectionManager.cs 73.62% <0.00%> (-2.20%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️
src/OpenTelemetry/Logs/Pool/LogRecordSharedPool.cs 100.00% <0.00%> (+2.63%) ⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

@cijothomas cijothomas merged commit a318130 into open-telemetry:main Mar 6, 2023
@CodeBlanch CodeBlanch deleted the resourcedetector-factory branch March 6, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants