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 ConfigureResource -> ConfigureResourceBuilder #3411

Conversation

alanwest
Copy link
Member

Staging this up based on open question here #3307 (comment).

@alanwest alanwest requested a review from a team June 27, 2022 21:16
@alanwest alanwest mentioned this pull request Jun 27, 2022
2 tasks
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #3411 (4f3e2ed) into main (5842a0a) will decrease coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3411      +/-   ##
==========================================
- Coverage   86.21%   86.05%   -0.16%     
==========================================
  Files         261      261              
  Lines        9411     9411              
==========================================
- Hits         8114     8099      -15     
- Misses       1297     1312      +15     
Impacted Files Coverage Δ
...c/OpenTelemetry/Logs/OpenTelemetryLoggerOptions.cs 78.57% <ø> (ø)
...elemetry/Metrics/MeterProviderBuilderExtensions.cs 63.82% <ø> (ø)
...Telemetry/Trace/TracerProviderBuilderExtensions.cs 90.90% <ø> (ø)
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 35.71% <0.00%> (-42.86%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 36.36% <0.00%> (-40.91%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 91.20% <0.00%> (-3.30%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 77.50% <0.00%> (+2.50%) ⬆️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 89.42% <0.00%> (+2.88%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+2.94%) ⬆️

alanwest added 2 commits June 27, 2022 15:37
…:alanwest/opentelemetry-dotnet into alanwest/rename-configureresourcebuilder
@Rs4178
Copy link

Rs4178 commented Jun 28, 2022

Staging this up based on open question here #3307 (comment).

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

I like this name better. Lets wait couple days to give more people to share feedbacks before merge.

@@ -39,7 +39,7 @@ private static object RunWithActivitySource()
// and use Console exporter.
using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddSource("Samples.SampleClient", "Samples.SampleServer")
.ConfigureResource(res => res.AddService("console-test"))
.ConfigureResourceBuilder(res => res.AddService("console-test"))
Copy link
Member

Choose a reason for hiding this comment

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

It seems the current code are bit random on the local variable naming.
Maybe use resourceBuilder or resBldr consistently across the repo?

@Oberon00
Copy link
Member

Oberon00 commented Jul 4, 2022

I liked the old/current name a bit more. Just as in ASP.NET one uses ConfigureServices, not ConfigureServiceCollection. Or this method here, another example where one uses a Builder to configure something but which doesn't have Builder in the name: https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.testing.webapplicationfactory-1.configurewebhost?view=aspnetcore-6.0 I could argue, I don't want to configure the builder, but use the builder to configure the resource(s) that will be created. But I do not have a very strong opinion.

@alanwest
Copy link
Member Author

alanwest commented Jul 6, 2022

I liked the old/current name a bit more. Just as in ASP.NET one uses ConfigureServices, not ConfigureServiceCollection. Or this method here, another example where one uses a Builder to configure something but which doesn't have Builder in the name: https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.testing.webapplicationfactory-1.configurewebhost?view=aspnetcore-6.0 I could argue, I don't want to configure the builder, but use the builder to configure the resource(s) that will be created. But I do not have a very strong opinion.

I agree, these are good examples and a good argument for keeping the name as is: ConfigureResource.

@cijothomas
Copy link
Member

I liked the old/current name a bit more. Just as in ASP.NET one uses ConfigureServices, not ConfigureServiceCollection. Or this method here, another example where one uses a Builder to configure something but which doesn't have Builder in the name: https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.testing.webapplicationfactory-1.configurewebhost?view=aspnetcore-6.0 I could argue, I don't want to configure the builder, but use the builder to configure the resource(s) that will be created. But I do not have a very strong opinion.

Thanks for explanation. It looks reasonable to me to keep existing name, and abort this PR.

@alanwest alanwest closed this Jul 7, 2022
@alanwest alanwest deleted the alanwest/rename-configureresourcebuilder branch July 7, 2022 20:41
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.

5 participants