Default Resource should have service.name attribute #1744

Austin-Tan · 2021-01-29T17:41:59Z

Changes

Instead of by default attaching a Telemetry SDK resource with package information, the default is attaching a one-tag service.name following the requirements specified in Resource.md.

Will have a follow-up PR to have exporters actually access this default.

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

CodeBlanch · 2021-01-29T17:50:34Z

src/OpenTelemetry/Resources/ResourceBuilder.cs

+        /// semantic conventions</a> for details.
+        /// </summary>
+        /// <returns>Created <see cref="ResourceBuilder"/>.</returns>
+        public static ResourceBuilder CreateSDK()


I think we should stick with the casing "Sdk" since it is used elsewhere (eg AddTelemetrySdk).

Also, do we need a CreateSdk? How will user know which one to call (CreateDefault() or CreatedSdk())? Probably better for us to give them just one path (pit of success).

What if we combined both into one thing?

public static ResourceBuilder CreateDefault(bool addTelemetrySdk = true)

Will fix the Sdk casing immediately. I like the idea of the optional parameter on default. This way we'll always have service.name and then optionally include the Telemetry SDK resource.

AddTelemetrySdk already exists on ResourceBuilder extensions, so why need it here again?

Is this not where we would provide access to the Telemetry Resource? We previously used CreateDefault to just link directly to AddTelemetrySDK

Users just need an API to add TelemetrySDK, which we already provide. There is no need of an additional method.
A user can do:

ResourceBuilder.CreateEmpty().AddTelemetrySdk() - Resource will only have telemetrysdk attributes.
or
ResourceBuilder.CreateDefault().AddTelemetrySdk() - Resource will have telemetrysdk attributes + the default. (default is service.name("unknownservice..."))
or
ResourceBuilder.CreateDefault().AddTelemetrySdk().AddServiceName("CijoService") - Resource will have telemetrysdk attributes + service.name ("CijoService"). Due the merge priority, "CijoService" overrides the default one.

This thinking makes a lot of sense to me. I'll remove the various extra things we introduced in this PR, and I like @CodeBlanch 's approach too but I think this is more elegant. Will not be implementing the optional parameter.

codecov · 2021-01-29T18:06:15Z

Codecov Report

Merging #1744 (d8d0d53) into main (416a6c1) will increase coverage by 0.99%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##             main    #1744      +/-   ##
==========================================
+ Coverage   83.14%   84.14%   +0.99%     
==========================================
  Files         193      187       -6     
  Lines        6005     5923      -82     
==========================================
- Hits         4993     4984       -9     
+ Misses       1012      939      -73

Impacted Files	Coverage Δ
...nTelemetry.Exporter.Jaeger/Implementation/Batch.cs	`84.09% <ø> (+3.65%)`	⬆️
...rc/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs	`83.33% <72.72%> (-1.93%)`	⬇️
src/OpenTelemetry/Resources/ResourceBuilder.cs	`92.85% <100.00%> (+2.38%)`	⬆️
...emetry.Exporter.Console/ConsoleActivityExporter.cs
...porter.Console/ConsoleExporterLoggingExtensions.cs
.../OpenTelemetry.Exporter.Console/ConsoleExporter.cs
...xporter.Console/ConsoleExporterHelperExtensions.cs
...lemetry.Exporter.Console/ConsoleExporterOptions.cs
...metry.Exporter.Console/ConsoleLogRecordExporter.cs
...rc/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs	`90.74% <0.00%> (+3.70%)`	⬆️
... and 2 more

src/OpenTelemetry/Resources/ResourceBuilderExtensions.cs

test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs

johnduhart · 2021-02-01T18:12:27Z

Instead of by default attaching a Telemetry SDK resource with package information, the default is attaching a one-tag service.name following the requirements specified in Resource.md.

I'm not sure I agree with this interpretation, the spec says "at least". Including the telemetry.* attributes by default seems like best-practice.

cijothomas · 2021-02-01T18:31:37Z

Including the telemetry.* attributes by default seems like best-practice.

Some exporters export all the resource attributes with every span. With this PR as is, If a user add Resource.Default - they'll get only the MUST have resource (servicename), instead of getting TelemetrySDK attributes as well.

But not opposed to making telemetrysdk to the Default itself. The spec about TelemetrySDK (https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/resource/semantic_conventions#telemetry-sdk), is not yet marked stable. So we should be able to do the following:

For 1.0.0, Resource.Default - only contain "service.name". If user want teleemtrySDK, they need to manually call AddTelemetrySdk.

Post 1.0.0, when Resource Semantic Conventions gets marked as stable, we can modify Resource.Default to include TelemetrySDK as well. (This shouldn't cause duplicates anyway, so would be backward compatible as well.)

Okay with this approach?

johnduhart · 2021-02-01T18:37:41Z

Okay, that makes sense.

…teDefault

cijothomas · 2021-02-01T18:54:43Z

src/OpenTelemetry/Resources/ResourceBuilderExtensions.cs

@@ -90,7 +90,7 @@ public static class ResourceBuilderExtensions
        /// <summary>
        /// Adds service information to a <see cref="ResourceBuilder"/>
        /// following <a
-        /// href="https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/resource/semantic_conventions#telemetry-sdk">semantic
+        /// href="https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource/semantic_conventions#telemetry-sdk">semantic


this change is incorrect. the branch was named to "main" last week.

I noticed this, but when I use .../master/... the link doesn't properly direct to the right part of the page. /main/ will show the warning but also direct to the right section of the page.
I'll still change it to master.

cijothomas · 2021-02-01T18:56:28Z

src/OpenTelemetry/CHANGELOG.md

@@ -44,6 +44,10 @@ Released 2021-Jan-29
  [#1501](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1655)
  for details on Metric release plans.
 * Fix Resource attribute telemetry.sdk.version to have correct file version.
+* Default `Resource` has been updated. No longer linked to Telemetry SDK,


I think a simple one liner like this is sufficient for changelog: Default resource will now contain service.name instead of telemetrysdk.

Fixed. Also the line above it, issue #1501 redirects to PR #1655. is this intended?

src/OpenTelemetry/Resources/ResourceBuilder.cs

Austin-Tan added 3 commits January 29, 2021 08:05

Moving the previous default (SDK info) into separate method

4517454

Add CreateDefault and add to API

493a855

Converting tests and adding actual default servicename

e5a5c91

Austin-Tan requested a review from a team January 29, 2021 17:42

CodeBlanch reviewed Jan 29, 2021

View reviewed changes

Fixing Tracer Test with SDKResoruce

7144e11

Austin-Tan added 4 commits January 29, 2021 13:29

removing api reference

aa9cfb6

merge main

86f9ffa

Fixing tests

6cef7fd

Changelog and API

bc22109

cijothomas reviewed Jan 30, 2021

View reviewed changes

src/OpenTelemetry/Resources/ResourceBuilderExtensions.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jan 30, 2021

View reviewed changes

test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs Show resolved Hide resolved

Austin-Tan added 2 commits January 31, 2021 21:48

Re-add test with default ProviderBuilder and privatize method

c0866b1

reverting API changes

ebad31c

Merge weith master

589499a

Removing AddDfault method altogether, performing addition inside Crea…

f76f9a1

…teDefault

cijothomas reviewed Feb 1, 2021

View reviewed changes

Austin-Tan added 3 commits February 1, 2021 10:59

Resolving comments - main instead of master, shortened changelog.md

667f3dc

Moved to unreleased

eb72c99

markdownlint fix (blank lines)

dc5f09b

Austin-Tan closed this Feb 1, 2021

Austin-Tan reopened this Feb 1, 2021

cijothomas reviewed Feb 1, 2021

View reviewed changes

src/OpenTelemetry/Resources/ResourceBuilder.cs Outdated Show resolved Hide resolved

Austin-Tan added 2 commits February 1, 2021 11:14

Closing brace should not be preceded by blank line

58f9e09

Removing colon from unknown_service when no processname

f985843

removing comma check from sdk test

12225c1

cijothomas reviewed Feb 1, 2021

View reviewed changes

src/OpenTelemetry/Resources/ResourceBuilder.cs Outdated Show resolved Hide resolved

Austin-Tan added 3 commits February 1, 2021 11:32

link to correct spec section

429a031

one other case of the colon missing

7e0be45

Correcting - reversing string.empty and processname in ternary

d8d0d53

cijothomas approved these changes Feb 1, 2021

View reviewed changes

cijothomas merged commit 18db60d into open-telemetry:main Feb 1, 2021

Austin-Tan deleted the DefaultResourceServiceName branch February 1, 2021 21:25

Austin-Tan mentioned this pull request Feb 1, 2021

Rely on DefaultResource's Service.Name for Exporters #1768

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default Resource should have service.name attribute #1744

Default Resource should have service.name attribute #1744

Austin-Tan commented Jan 29, 2021

CodeBlanch Jan 29, 2021

CodeBlanch Jan 29, 2021

Austin-Tan Jan 29, 2021

cijothomas Jan 29, 2021

Austin-Tan Jan 29, 2021

cijothomas Jan 29, 2021 •

edited

Loading

Austin-Tan Jan 29, 2021

codecov bot commented Jan 29, 2021 •

edited

Loading

johnduhart commented Feb 1, 2021

cijothomas commented Feb 1, 2021

johnduhart commented Feb 1, 2021

cijothomas Feb 1, 2021

Austin-Tan Feb 1, 2021

cijothomas Feb 1, 2021

Austin-Tan Feb 1, 2021

Default Resource should have service.name attribute #1744

Default Resource should have service.name attribute #1744

Conversation

Austin-Tan commented Jan 29, 2021

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2021 • edited Loading

Codecov Report

johnduhart commented Feb 1, 2021

cijothomas commented Feb 1, 2021

johnduhart commented Feb 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas Jan 29, 2021 •

edited

Loading

codecov bot commented Jan 29, 2021 •

edited

Loading