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 Resource public ctor as Provider expects ResourceBuilder now #1566

Merged
merged 6 commits into from
Nov 17, 2020

Conversation

cijothomas
Copy link
Member

Fixes #.

Changes

Please provide a brief description of the changes here.

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

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@cijothomas cijothomas requested a review from a team November 17, 2020 16:14
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

I left the ctor public for two reasons:

  1. Spec specifically calls out a create method on Resource that accepts attributes. The ctor satisfied that nicely. Really Resource class is highly compliant with the spec.
  2. You can new up a Resource and pass it to the AddResource method on ResourceBuilder. User is free to do this, if they want to:
    var builder = ResourceBuilder.CreateEmpty()
       .AddService("MyService")
       .AddResource(new Resource(new Dictionary<string, object> { ["Key1"] = "Value1" }))
       .AddResource(new Resource(new Dictionary<string, object> { ["Key2"] = "Value2" }));

If you still want to make internal though, fine with me.

@cijothomas
Copy link
Member Author

I left the ctor public for two reasons:

  1. Spec specifically calls out a create method on Resource that accepts attributes. The ctor satisfied that nicely. Really Resource class is highly compliant with the spec.
  2. You can new up a Resource and pass it to the AddResource method on ResourceBuilder. User is free to do this, if they want to:
    var builder = ResourceBuilder.CreateEmpty()
       .AddService("MyService")
       .AddResource(new Resource(new Dictionary<string, object> { ["Key1"] = "Value1" }))
       .AddResource(new Resource(new Dictionary<string, object> { ["Key2"] = "Value2" }));

If you still want to make internal though, fine with me.

Can I also remove builder.AddResource(Resource) as well? That'll ensure there is only way to create Resource - its through ResourceBuilder.

@CodeBlanch
Copy link
Member

@cijothomas

Can I also remove builder.AddResource(Resource) as well? That'll ensure there is only way to create Resource - its through ResourceBuilder.

Check out ResourceBuilderExtensions.cs. All the extensions use AddResource to add things to the builder. The Build() call just loops through the added Resources and calls Merge on them. You could make AddResource internal (like AddDetector), but then extension authors wouldn't be able to extend the API without some kind of reflection. May or may not be an issue?

@cijothomas
Copy link
Member Author

@cijothomas

Can I also remove builder.AddResource(Resource) as well? That'll ensure there is only way to create Resource - its through ResourceBuilder.

Check out ResourceBuilderExtensions.cs. All the extensions use AddResource to add things to the builder. The Build() call just loops through the added Resources and calls Merge on them. You could make AddResource internal (like AddDetector), but then extension authors wouldn't be able to extend the API without some kind of reflection. May or may not be an issue?

The extensibility for Resource would come through ResourceDetectors - we just need to wait for spec to finalize on that. (https://github.com/open-telemetry/oteps/blob/master/text/0111-auto-resource-detection.md)

But, until then, we can still do the below, right?

public static ResourceBuilder AddCijoResource(this ResourceBuilder resourceBuilder)
        {
            // var attributes = populate Cijo Resources;
            return ResourceBuilder.AddAttributes(attributes);
        }

@CodeBlanch
Copy link
Member

@cijothomas

But, until then, we can still do the below, right?

Ya that would work fine, good call.

@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #1566 (d6dc645) into master (68adae6) will increase coverage by 0.05%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1566      +/-   ##
==========================================
+ Coverage   81.00%   81.05%   +0.05%     
==========================================
  Files         237      237              
  Lines        6453     6449       -4     
==========================================
  Hits         5227     5227              
+ Misses       1226     1222       -4     
Impacted Files Coverage Δ
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <ø> (ø)
...Telemetry/Trace/TracerProviderBuilderExtensions.cs 70.58% <ø> (ø)
...rc/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs 87.03% <ø> (ø)
src/OpenTelemetry/Resources/ResourceBuilder.cs 90.47% <75.00%> (ø)
...try.Exporter.OpenTelemetryProtocol/OtlpExporter.cs 57.89% <100.00%> (-0.65%) ⬇️
src/OpenTelemetry/Resources/Resource.cs 93.33% <100.00%> (ø)
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 21.68% <0.00%> (+3.61%) ⬆️

@cijothomas cijothomas merged commit 7a91923 into master Nov 17, 2020
@cijothomas cijothomas deleted the cijothomas/resourcefix10 branch November 17, 2020 22:26
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.

2 participants