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.

Sign up for GitHub

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

Default Resource should have service.name attribute #1744

Merged
merged 21 commits into from
Feb 1, 2021
Merged

Default Resource should have service.name attribute #1744

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4517454
Moving the previous default (SDK info) into separate method
Austin-Tan Jan 29, 2021
493a855
Add CreateDefault and add to API
Austin-Tan Jan 29, 2021
e5a5c91
Converting tests and adding actual default servicename
Austin-Tan Jan 29, 2021
7144e11
Fixing Tracer Test with SDKResoruce
Austin-Tan Jan 29, 2021
aa9cfb6
removing api reference
Austin-Tan Jan 29, 2021
86f9ffa
merge main
Austin-Tan Jan 29, 2021
6cef7fd
Fixing tests
Austin-Tan Jan 29, 2021
bc22109
Changelog and API
Austin-Tan Jan 29, 2021
c0866b1
Re-add test with default ProviderBuilder and privatize method
Austin-Tan Feb 1, 2021
ebad31c
reverting API changes
Austin-Tan Feb 1, 2021
589499a
Merge weith master
Austin-Tan Feb 1, 2021
f76f9a1
Removing AddDfault method altogether, performing addition inside Crea…
Austin-Tan Feb 1, 2021
667f3dc
Resolving comments - main instead of master, shortened changelog.md
Austin-Tan Feb 1, 2021
eb72c99
Moved to unreleased
Austin-Tan Feb 1, 2021
dc5f09b
markdownlint fix (blank lines)
Austin-Tan Feb 1, 2021
58f9e09
Closing brace should not be preceded by blank line
Austin-Tan Feb 1, 2021
f985843
Removing colon from unknown_service when no processname
Austin-Tan Feb 1, 2021
12225c1
removing comma check from sdk test
Austin-Tan Feb 1, 2021
429a031
link to correct spec section
Austin-Tan Feb 1, 2021
7e0be45
one other case of the colon missing
Austin-Tan Feb 1, 2021
d8d0d53
Correcting - reversing string.empty and processname in ternary
Austin-Tan Feb 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/OpenTelemetry/.publicApi/net452/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ abstract OpenTelemetry.BaseExportProcessor<T>.OnExport(T data) -> void
override sealed OpenTelemetry.BaseExportProcessor<T>.OnStart(T data) -> void
readonly OpenTelemetry.BaseExportProcessor<T>.exporter -> OpenTelemetry.BaseExporter<T>
static OpenTelemetry.ProviderExtensions.GetResource(this OpenTelemetry.BaseProvider baseProvider) -> OpenTelemetry.Resources.Resource
static OpenTelemetry.Resources.ResourceBuilder.CreateSDK() -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddDefault(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder) -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddEnvironmentVariableDetector(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder) -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.Resource.Empty.get -> OpenTelemetry.Resources.Resource
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddAttributes(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object>> attributes) -> OpenTelemetry.Resources.ResourceBuilder
Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry/.publicApi/net46/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ abstract OpenTelemetry.BaseExportProcessor<T>.OnExport(T data) -> void
override sealed OpenTelemetry.BaseExportProcessor<T>.OnStart(T data) -> void
readonly OpenTelemetry.BaseExportProcessor<T>.exporter -> OpenTelemetry.BaseExporter<T>
static OpenTelemetry.ProviderExtensions.GetResource(this OpenTelemetry.BaseProvider baseProvider) -> OpenTelemetry.Resources.Resource
static OpenTelemetry.Resources.ResourceBuilder.CreateSDK() -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddDefault(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder) -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddEnvironmentVariableDetector(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder) -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.Resource.Empty.get -> OpenTelemetry.Resources.Resource
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddAttributes(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object>> attributes) -> OpenTelemetry.Resources.ResourceBuilder
Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,8 @@ override sealed OpenTelemetry.BaseExportProcessor<T>.OnStart(T data) -> void
readonly OpenTelemetry.BaseExportProcessor<T>.exporter -> OpenTelemetry.BaseExporter<T>
static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.AddOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder builder, System.Action<OpenTelemetry.Logs.OpenTelemetryLoggerOptions> configure = null) -> Microsoft.Extensions.Logging.ILoggingBuilder
static OpenTelemetry.ProviderExtensions.GetResource(this OpenTelemetry.BaseProvider baseProvider) -> OpenTelemetry.Resources.Resource
static OpenTelemetry.Resources.ResourceBuilder.CreateSDK() -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddDefault(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder) -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddEnvironmentVariableDetector(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder) -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.Resource.Empty.get -> OpenTelemetry.Resources.Resource
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddAttributes(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object>> attributes) -> OpenTelemetry.Resources.ResourceBuilder
Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,8 @@ override sealed OpenTelemetry.BaseExportProcessor<T>.OnStart(T data) -> void
readonly OpenTelemetry.BaseExportProcessor<T>.exporter -> OpenTelemetry.BaseExporter<T>
static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.AddOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder builder, System.Action<OpenTelemetry.Logs.OpenTelemetryLoggerOptions> configure = null) -> Microsoft.Extensions.Logging.ILoggingBuilder
static OpenTelemetry.ProviderExtensions.GetResource(this OpenTelemetry.BaseProvider baseProvider) -> OpenTelemetry.Resources.Resource
static OpenTelemetry.Resources.ResourceBuilder.CreateSDK() -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddDefault(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder) -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddEnvironmentVariableDetector(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder) -> OpenTelemetry.Resources.ResourceBuilder
static OpenTelemetry.Resources.Resource.Empty.get -> OpenTelemetry.Resources.Resource
static OpenTelemetry.Resources.ResourceBuilderExtensions.AddAttributes(this OpenTelemetry.Resources.ResourceBuilder resourceBuilder, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object>> attributes) -> OpenTelemetry.Resources.ResourceBuilder
Expand Down
16 changes: 13 additions & 3 deletions src/OpenTelemetry/Resources/ResourceBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,23 @@ private ResourceBuilder()
}

/// <summary>
/// Creates a <see cref="ResourceBuilder"/> instance with SDK defaults
/// added. See <a
/// href="https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/">resource
/// Creates a <see cref="ResourceBuilder"/> instance with Default
/// service.name added. See <a
/// href="https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource/semantic_conventions#service/">resource
Austin-Tan marked this conversation as resolved.
Show resolved Hide resolved
/// semantic conventions</a> for details.
/// </summary>
/// <returns>Created <see cref="ResourceBuilder"/>.</returns>
public static ResourceBuilder CreateDefault()
=> new ResourceBuilder().AddDefault();

/// <summary>
/// Creates a <see cref="ResourceBuilder"/> instance with Telemetry
/// SDK added. See <a
/// href="https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource/semantic_conventions#telemetry-sdk/">resource
/// semantic conventions</a> for details.
/// </summary>
/// <returns>Created <see cref="ResourceBuilder"/>.</returns>
public static ResourceBuilder CreateSDK()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@cijothomas cijothomas Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

=> new ResourceBuilder().AddTelemetrySdk(); // TODO: Seek spec clarify on whether or not OtelEnvResourceDetector should be added by default.

/// <summary>
Expand Down
19 changes: 19 additions & 0 deletions src/OpenTelemetry/Resources/ResourceBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ public static class ResourceBuilderExtensions
{
private static readonly Version Version = typeof(Resource).Assembly.GetName().Version;

private static Resource DefaultResource { get; } = new Resource(new Dictionary<string, object>
{
[ResourceSemanticConventions.AttributeServiceName] = "unknown_service:"
+ (string.IsNullOrWhiteSpace(System.Diagnostics.Process.GetCurrentProcess().ProcessName) ? System.Diagnostics.Process.GetCurrentProcess().ProcessName : string.Empty),
});

private static Resource TelemetryResource { get; } = new Resource(new Dictionary<string, object>
{
[ResourceSemanticConventions.AttributeTelemetrySdkName] = "opentelemetry",
Expand Down Expand Up @@ -86,6 +92,19 @@ public static ResourceBuilder AddService(
return resourceBuilder.AddResource(new Resource(resourceAttributes));
}

/// <summary>
/// Adds service information to a <see cref="ResourceBuilder"/>
/// following <a
/// href="https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource/semantic_conventions#service">semantic
/// conventions</a>.
/// </summary>
/// <param name="resourceBuilder"><see cref="ResourceBuilder"/>.</param>
/// <returns>Returns <see cref="ResourceBuilder"/> for chaining.</returns>
public static ResourceBuilder AddDefault(this ResourceBuilder resourceBuilder)
Austin-Tan marked this conversation as resolved.
Show resolved Hide resolved
{
return resourceBuilder.AddResource(DefaultResource);
}

/// <summary>
/// Adds service information to a <see cref="ResourceBuilder"/>
/// following <a
Expand Down
31 changes: 25 additions & 6 deletions test/OpenTelemetry.Tests/Resources/ResourceTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,17 +331,29 @@ public void MergeResource_UpdatingResourceOverridesCurrentResource()
}

[Fact]
public void GetResourceWithDefaultAttributes_EmptyResource()
public void GetResourceWithTelemetrySDKAttributes()
{
// Arrange
var resource = ResourceBuilder.CreateDefault().AddEnvironmentVariableDetector().Build();
var resource = ResourceBuilder.CreateSDK().AddEnvironmentVariableDetector().Build();

// Assert
var attributes = resource.Attributes;
Assert.Equal(3, attributes.Count());
ValidateTelemetrySdkAttributes(attributes);
}

[Fact]
public void GetResourceWithDefaultAttributes_EmptyResource()
{
// Arrange
var resource = ResourceBuilder.CreateDefault().AddEnvironmentVariableDetector().Build();

// Assert
var attributes = resource.Attributes;
Assert.Single(attributes);
ValidateDefaultAttributes(attributes);
}

[Fact]
public void GetResourceWithDefaultAttributes_ResourceWithAttrs()
{
Expand All @@ -350,9 +362,9 @@ public void GetResourceWithDefaultAttributes_ResourceWithAttrs()

// Assert
var attributes = resource.Attributes;
Assert.Equal(5, attributes.Count());
Assert.Equal(3, attributes.Count());
ValidateAttributes(attributes, 0, 1);
ValidateTelemetrySdkAttributes(attributes);
ValidateDefaultAttributes(attributes);
}

[Fact]
Expand All @@ -364,9 +376,9 @@ public void GetResourceWithDefaultAttributes_WithEnvVar()

// Assert
var attributes = resource.Attributes;
Assert.Equal(7, attributes.Count());
Assert.Equal(5, attributes.Count());
ValidateAttributes(attributes, 0, 1);
ValidateTelemetrySdkAttributes(attributes);
ValidateDefaultAttributes(attributes);
Assert.Contains(new KeyValuePair<string, object>("EVKey1", "EVVal1"), attributes);
Assert.Contains(new KeyValuePair<string, object>("EVKey2", "EVVal2"), attributes);
}
Expand Down Expand Up @@ -412,6 +424,13 @@ private static void ValidateTelemetrySdkAttributes(IEnumerable<KeyValuePair<stri
Assert.Single(versionAttribute);
}

private static void ValidateDefaultAttributes(IEnumerable<KeyValuePair<string, object>> attributes)
{
var serviceName = attributes.Where(pair => pair.Key.Equals("service.name"));
Assert.Single(serviceName);
Assert.Contains("unknown_service:", serviceName.FirstOrDefault().Value as string);
}

private Dictionary<string, object> CreateAttributes(int attributeCount, int startIndex = 0)
{
var attributes = new Dictionary<string, object>();
Expand Down