From ad183bc9f7a8fc460b757becf346aa8399e10b8d Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 10 Jun 2020 18:56:47 -0700 Subject: [PATCH 01/14] Improve Activity API usability and OpenTelemetry integration (Part 2) --- .../diagnostics/activity-improvements-2.md | 156 ++++++++++++++++++ 1 file changed, 156 insertions(+) create mode 100644 accepted/2020/diagnostics/activity-improvements-2.md diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md new file mode 100644 index 000000000..ab5718ac6 --- /dev/null +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -0,0 +1,156 @@ +# Improve Activity API usability and OpenTelemetry integration (Part 2) + +We have exposed some [Activity APIs to improve the API usability and to integrate with OpenTelemetry](https://github.com/dotnet/designs/pull/98) (OT) so Activity can be a suitable replacement for the OT Span type described by the [OT Specs](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span). We have received feedback regarding some OT blocked scenarios which need to be addressed in the Activity APIs and unblock the OT SDK implementation. + +This document is listing the proposed changes and the rationale for the proposal. **As we still receiving more feedback and there are some issues under discussion, it is expected this documents will get updated before we do the full design review for the proposed changes.** + +## ActivityContext.IsRemote + +We already exposed the ActivityContext type as described by the OT specs. There is a property inside this type called `IsRemote` which we didn't include in the first place because it was under discussion with the OT spec committee. It has been confirmed this property is needed and will stay as part of the OT specs. + +[ActivityContext.IsRemote](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#spancontext) is a Boolean flag which returns true if the ActivityContext was propagated from a remote parent. When creating children from remote spans, their IsRemote flag MUST be set to false. + +```c# + public readonly partial struct ActivityContext : IEquatable + { + // This constructor already exist but we are adding extra defaulted parameter for isRemote. + public ActivityContext(ActivityTraceId traceId, ActivitySpanId spanId, ActivityTraceFlags traceFlags, string? traceState = null, isRomte = false) {...} + + // This is the new property + public bool IsRemote { get; } + } +``` + +Activity class will need to expose such property too to be able to set it as part of the included context. This is needed by the OT asp.net and Http client OT adapters. + +```c# + public partial class Activity : IDisposable + { + public bool IsRemote { get; set; } + } +``` + +## Activity.Kind Setter + +Activity.Kind is already exposed property but we have exposed it as a read-only property. We got OT adapters scenario that listen to the old created activities (by asp.net and Http clients). As the old Activities didn't support Kind property, the adapter needs to set this property before sending the activity to the sampling in the OT SDK. The proposal here is to expose the setter of this property. + +```c# + public partial class Activity : IDisposable + { + // The setter was private and now we are making it public + public ActivityKind Kind { get; set; } = ActivityKind.Internal; + } +``` + +## ActivityTagsCollection + +OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. the value can be numeric, bool, string types. Also, the value can be an array of one of these basic types. + +> ```json +> 'x-forwarded-for': 'foo.bar' +> 'timeout': 5 +> ``` + +The attributes can be nested too. There is opened [OT spec PR](https://github.com/open-telemetry/opentelemetry-specification/pull/596) for supporting this scenario. + +> ```json +> 'http': { +> 'headers': [ +> 'x-forwarded-for': 'foo.bar' +> 'keep-alive': 'timeout=5' +> ] +> } +> ``` + +The attributes are used in multiple places in the OT (e.g. in Span, Event, and Link). Activity support Tags which similar concept but Tags is limited to string value types and not supporting any other types `IEnumerable> Tags`. We'll need to extend the Tags concept in our APIs to support the OT scenarios. We'll do that by introducing the ActivityTagsCollection and extending Activity class to support adding different types of tags value. + +ActivityTagsCollection is needed for the following reasons: + +- We need to avoid having the consumers of our APIs to use any other types (e.g. Dictionary<>, List<>...etc.) which can cause issues like not keeping the items in order or not conforming to the OT specs which suggest specific behaviors (e.g. when adding null value, should remove the entry from the list) +- Need to support the nested attributes. ActivityTagsCollection will make it easy to do so. + +```c# + public class ActivityTagsCollection : IReadOnlyCollection> + { + public ActivityTagsCollection() { } + public ActivityTagsCollection(IEnumerable>) { } + + public void Add(string key, int value) { } + public void Add(string key, double value) { } + public void Add(string key, bool value) { } + public void Add(string key, string value) { } + public void Add(string key, int [] value) { } + public void Add(string key, double [] value) { } + public void Add(string key, bool [] value) { } + public void Add(string key, string [] value) { } + public void Add(string key, ActivityTagsCollection value) { } + + // interfaces implementation + public int Count { get; } + public IEnumerator> GetEnumerator() { ... } + IEnumerator IEnumerable.GetEnumerator() { ... } + } +``` + +## Activity.Tags + +As mentioned earlier, Activity has Tags property which returns the list of the activity tags. Tags are a list of `KeyValuePair`. Tags are proven to be not enough for OT scenarios as we need to support more value types (numeric, bool, numeric array, and nested tags). Activity today has the method `public Activity AddTag(string key, string? value)`. The proposal here is to add more `AddTag` overload accepting more types and add a new getter property return the list of all typed tags. + +```C# + public partial class Activity : IDisposable + { + public void AddTag(string key, int value) { } + public void AddTag(string key, double value) { } + public void AddTag(string key, bool value) { } + public void AddTag(string key, int [] value) { } + public void AddTag(string key, double [] value) { } + public void AddTag(string key, bool [] value) { } // how interesting this one? + public void AddTag(string key, string [] value) { } + public void AddTag(string key, ActivityTagsCollection value) { } + + // We cannot change Tags property so we are introducing a new property with a new name + public ActivityTagsCollection TypedTags { get; } + } +``` + +## Proposal Changing already exposed APIs in previous preview releases + +As we are introducing the ActivityTagsCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. + +### ActivityEvent + +```C# + // Already exposed APIs + public readonly struct ActivityEvent + { + public ActivityEvent(string name, IEnumerable>? attributes) + public ActivityEvent(string name, DateTimeOffset timestamp, IEnumerable>? attributes) + public IEnumerable> Attributes { get; } + } + + // change it to the following: + public readonly struct ActivityEvent + { + public ActivityEvent(string name, ActivityTagsCollection? attributes) + public ActivityEvent(string name, DateTimeOffset timestamp, ActivityTagsCollection? attributes) + public ActivityTagsCollection? Attributes { get; } + } +``` + +### ActivityLink + +```C# + // Already exposed APIs + public readonly partial struct ActivityLink : IEquatable + { + public ActivityLink(ActivityContext context, IEnumerable>? attributes) + public IEnumerable>? Attributes { get; } + } + + // change it to the following: + public readonly partial struct ActivityLink : IEquatable + { + public ActivityLink(ActivityContext context, ActivityTagsCollection? attributes) + public ActivityTagsCollection? Attributes { get; } + } +``` \ No newline at end of file From 173a65673a340303d6ef4c77e315ca35a55a1226 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 10 Jun 2020 20:18:32 -0700 Subject: [PATCH 02/14] Update accepted/2020/diagnostics/activity-improvements-2.md Co-authored-by: Reiley Yang --- accepted/2020/diagnostics/activity-improvements-2.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index ab5718ac6..4c0be9a0e 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -44,7 +44,7 @@ Activity.Kind is already exposed property but we have exposed it as a read-only ## ActivityTagsCollection -OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. the value can be numeric, bool, string types. Also, the value can be an array of one of these basic types. +OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. The value can be numeric, bool, string types. Also, the value can be an array of one of these basic types. > ```json > 'x-forwarded-for': 'foo.bar' @@ -153,4 +153,4 @@ As we are introducing the ActivityTagsCollection, we can take advantage of that public ActivityLink(ActivityContext context, ActivityTagsCollection? attributes) public ActivityTagsCollection? Attributes { get; } } -``` \ No newline at end of file +``` From 3d60f670a05301d23184f7737ab38d70b1bb5385 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 11 Jun 2020 10:03:55 -0700 Subject: [PATCH 03/14] Address more feedback --- accepted/2020/diagnostics/activity-improvements-2.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index 4c0be9a0e..30f6a280e 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -115,7 +115,7 @@ As mentioned earlier, Activity has Tags property which returns the list of the a ## Proposal Changing already exposed APIs in previous preview releases -As we are introducing the ActivityTagsCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. +As we are introducing the ActivityTagsCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. Also, we are renaming Attributes to Tags for consistency inside BCL APIs. ### ActivityEvent @@ -131,9 +131,9 @@ As we are introducing the ActivityTagsCollection, we can take advantage of that // change it to the following: public readonly struct ActivityEvent { - public ActivityEvent(string name, ActivityTagsCollection? attributes) - public ActivityEvent(string name, DateTimeOffset timestamp, ActivityTagsCollection? attributes) - public ActivityTagsCollection? Attributes { get; } + public ActivityEvent(string name, ActivityTagsCollection? tags) + public ActivityEvent(string name, DateTimeOffset timestamp, ActivityTagsCollection? tags) + public ActivityTagsCollection? Tags { get; } } ``` @@ -150,7 +150,7 @@ As we are introducing the ActivityTagsCollection, we can take advantage of that // change it to the following: public readonly partial struct ActivityLink : IEquatable { - public ActivityLink(ActivityContext context, ActivityTagsCollection? attributes) - public ActivityTagsCollection? Attributes { get; } + public ActivityLink(ActivityContext context, ActivityTagsCollection? tags) + public ActivityTagsCollection? Tags { get; } } ``` From 4fbe482d7257714b79bc7341714daf89f141b25c Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 11 Jun 2020 10:13:51 -0700 Subject: [PATCH 04/14] added open question --- accepted/2020/diagnostics/activity-improvements-2.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index 30f6a280e..5f4eccd28 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -113,6 +113,15 @@ As mentioned earlier, Activity has Tags property which returns the list of the a } ``` +***OpenQuestion*** + +We need to decide about the behavior of the old property `Activity.Tags` when adding tags of non-string types values. The options are + +- Ignore all non-string typed values and return only the string type value tags. +- Convert the non-string type values to string before returning it. + +I am inclining to the first option to avoid handling ToString with more complex types (e.g. arrays or ActivityTagsCollection). + ## Proposal Changing already exposed APIs in previous preview releases As we are introducing the ActivityTagsCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. Also, we are renaming Attributes to Tags for consistency inside BCL APIs. From d3552bd6ed7f6699d45c4d76064c743759ff7809 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 11 Jun 2020 12:55:15 -0700 Subject: [PATCH 05/14] Update accepted/2020/diagnostics/activity-improvements-2.md Co-authored-by: Paulo Janotti --- accepted/2020/diagnostics/activity-improvements-2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index 5f4eccd28..7b6b51d1b 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -14,7 +14,7 @@ We already exposed the ActivityContext type as described by the OT specs. There public readonly partial struct ActivityContext : IEquatable { // This constructor already exist but we are adding extra defaulted parameter for isRemote. - public ActivityContext(ActivityTraceId traceId, ActivitySpanId spanId, ActivityTraceFlags traceFlags, string? traceState = null, isRomte = false) {...} + public ActivityContext(ActivityTraceId traceId, ActivitySpanId spanId, ActivityTraceFlags traceFlags, string? traceState = null, isRemote = false) {...} // This is the new property public bool IsRemote { get; } From 11b04f431710cdb0ac78b5f0ee5169d49eb89d65 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 11 Jun 2020 18:32:41 -0700 Subject: [PATCH 06/14] Added Trace Id item to the list --- .../diagnostics/activity-improvements-2.md | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index 7b6b51d1b..d20fd59ba 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -15,13 +15,13 @@ We already exposed the ActivityContext type as described by the OT specs. There { // This constructor already exist but we are adding extra defaulted parameter for isRemote. public ActivityContext(ActivityTraceId traceId, ActivitySpanId spanId, ActivityTraceFlags traceFlags, string? traceState = null, isRemote = false) {...} - - // This is the new property + + // This is the new property public bool IsRemote { get; } } ``` -Activity class will need to expose such property too to be able to set it as part of the included context. This is needed by the OT asp.net and Http client OT adapters. +Activity class will need to expose such property too to be able to set it as part of the included context. This is needed by the OT asp.net and Http client OT adapters. The adapters listen to the Activities created old way using `new Activity(...)` as it happen in asp.net and Http client. That means such activities will never set IsRemote correctly as it doesn't know anything about it. The adapters will need detect if such activity created from remote parent and set this value accordingly. ```c# public partial class Activity : IDisposable @@ -44,14 +44,14 @@ Activity.Kind is already exposed property but we have exposed it as a read-only ## ActivityTagsCollection -OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. The value can be numeric, bool, string types. Also, the value can be an array of one of these basic types. +OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. The value can be numeric, bool, string types. Also, the value can be an array of one of these basic types. > ```json > 'x-forwarded-for': 'foo.bar' > 'timeout': 5 > ``` -The attributes can be nested too. There is opened [OT spec PR](https://github.com/open-telemetry/opentelemetry-specification/pull/596) for supporting this scenario. +There is OT proposal tracked by the [PR](https://github.com/open-telemetry/opentelemetry-specification/pull/596) to support the nested attributes. If this get accepted in OT, it will be easy to support it using ActivityTagsCollection. > ```json > 'http': { @@ -66,8 +66,8 @@ The attributes are used in multiple places in the OT (e.g. in Span, Event, and L ActivityTagsCollection is needed for the following reasons: -- We need to avoid having the consumers of our APIs to use any other types (e.g. Dictionary<>, List<>...etc.) which can cause issues like not keeping the items in order or not conforming to the OT specs which suggest specific behaviors (e.g. when adding null value, should remove the entry from the list) -- Need to support the nested attributes. ActivityTagsCollection will make it easy to do so. +- We need to avoid having the consumers of our APIs to use any other types (e.g. Dictionary<>, List<>...etc.) which can cause issues like not keeping the items in order or not conforming to the OT specs which suggest specific behaviors (e.g. when adding null value, should remove the entry from the list) +- It is currently proposed to support nested attributes. ActivityTagsCollection will make it easy to do so at anytime is needed. ```c# public class ActivityTagsCollection : IReadOnlyCollection> @@ -83,6 +83,8 @@ ActivityTagsCollection is needed for the following reasons: public void Add(string key, double [] value) { } public void Add(string key, bool [] value) { } public void Add(string key, string [] value) { } + + // The following API is pending the approval in OT spec first before we add it in .NET. public void Add(string key, ActivityTagsCollection value) { } // interfaces implementation @@ -94,7 +96,7 @@ ActivityTagsCollection is needed for the following reasons: ## Activity.Tags -As mentioned earlier, Activity has Tags property which returns the list of the activity tags. Tags are a list of `KeyValuePair`. Tags are proven to be not enough for OT scenarios as we need to support more value types (numeric, bool, numeric array, and nested tags). Activity today has the method `public Activity AddTag(string key, string? value)`. The proposal here is to add more `AddTag` overload accepting more types and add a new getter property return the list of all typed tags. +As mentioned earlier, Activity has Tags property which returns the list of the activity tags. Tags are a list of `KeyValuePair`. Tags are proven to be not enough for OT scenarios as we need to support more value types (numeric, bool, numeric array, and nested tags). Activity today has the method `public Activity AddTag(string key, string? value)`. The proposal here is to add more `AddTag` overload accepting more types and add a new getter property return the list of all typed tags. ```C# public partial class Activity : IDisposable @@ -106,6 +108,8 @@ As mentioned earlier, Activity has Tags property which returns the list of the a public void AddTag(string key, double [] value) { } public void AddTag(string key, bool [] value) { } // how interesting this one? public void AddTag(string key, string [] value) { } + + // The following API is pending the approval in OT spec first before we add it in .NET. public void AddTag(string key, ActivityTagsCollection value) { } // We cannot change Tags property so we are introducing a new property with a new name @@ -120,7 +124,7 @@ We need to decide about the behavior of the old property `Activity.Tags` when ad - Ignore all non-string typed values and return only the string type value tags. - Convert the non-string type values to string before returning it. -I am inclining to the first option to avoid handling ToString with more complex types (e.g. arrays or ActivityTagsCollection). +I am inclining to the first option to avoid handling ToString with more complex types (e.g. arrays or ActivityTagsCollection). ## Proposal Changing already exposed APIs in previous preview releases @@ -163,3 +167,19 @@ As we are introducing the ActivityTagsCollection, we can take advantage of that public ActivityTagsCollection? Tags { get; } } ``` + +## Automatic Trace Id generation in case of null parent + +Sampler like probabilistic sampler can depend on the trace id for the sampling decision. Usually before creating any Activity using ActivitySource, the `ActivityListener.GetRequestedDataUsingContext` callback will get called which usually get implemented by the samplers. The parent context is sent as a parameter to this callback and the sampler gets the trace id from this parent context to perform the sampling action. When there is no parent, we send the default context which is nulling the trace id. The samplers depending on trace id are not going to be able to do the sampling at that time. The samplers even cannot generate and use a trace id because it cannot inform the Activity creation process to use the generated trace Id when creating the Activity object. +There are a couple of proposals how to address this issue in the .NET runtime. The idea is the runtime need to generate the trace Id and send it to the samplers. If the sampler using the passed trace id and decide to sample in this case, the Activity which will get created because of that sampler decision will be using the generated trace Id in its context. + +Solution options: + +- Before calling `ActivityListener.GetRequestedDataUsingContext`, there will be a check if we don't have a parent, then we generate a random trace Id and use it in the context sent to the callback. If decided to sample in, we all use this trace Id during the Activity object creation. This option doesn't need any API changes and will be fully handled inside the implementation. +- Add a new property to the `ActivityCreationOptions` struct (e.g. `NullParentTraceId`) which will carry the trace id that can be used by the sampler when detecting no parent context. obviously this will need API change. + +I am inclining to the first solution for the following reasons: + +- Exposing extra property can confuse the API consumers to know exactly when this property is needed or even they should care about it. +- Adding such property to `ActivityCreationOptions` will increase the size of this struct which we are trying to make it compact for perf reasons. +- I am not seeing a strong reason why the first solution wouldn't be good enough. From df88afa9df3b9cd0215624380b5a41a447cc3c79 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 23 Jun 2020 12:44:46 -0700 Subject: [PATCH 07/14] Address the feedback --- .../diagnostics/activity-improvements-2.md | 108 ++++++------------ 1 file changed, 34 insertions(+), 74 deletions(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index d20fd59ba..b6902b0e6 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -2,8 +2,6 @@ We have exposed some [Activity APIs to improve the API usability and to integrate with OpenTelemetry](https://github.com/dotnet/designs/pull/98) (OT) so Activity can be a suitable replacement for the OT Span type described by the [OT Specs](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span). We have received feedback regarding some OT blocked scenarios which need to be addressed in the Activity APIs and unblock the OT SDK implementation. -This document is listing the proposed changes and the rationale for the proposal. **As we still receiving more feedback and there are some issues under discussion, it is expected this documents will get updated before we do the full design review for the proposed changes.** - ## ActivityContext.IsRemote We already exposed the ActivityContext type as described by the OT specs. There is a property inside this type called `IsRemote` which we didn't include in the first place because it was under discussion with the OT spec committee. It has been confirmed this property is needed and will stay as part of the OT specs. @@ -21,15 +19,6 @@ We already exposed the ActivityContext type as described by the OT specs. There } ``` -Activity class will need to expose such property too to be able to set it as part of the included context. This is needed by the OT asp.net and Http client OT adapters. The adapters listen to the Activities created old way using `new Activity(...)` as it happen in asp.net and Http client. That means such activities will never set IsRemote correctly as it doesn't know anything about it. The adapters will need detect if such activity created from remote parent and set this value accordingly. - -```c# - public partial class Activity : IDisposable - { - public bool IsRemote { get; set; } - } -``` - ## Activity.Kind Setter Activity.Kind is already exposed property but we have exposed it as a read-only property. We got OT adapters scenario that listen to the old created activities (by asp.net and Http clients). As the old Activities didn't support Kind property, the adapter needs to set this property before sending the activity to the sampling in the OT SDK. The proposal here is to expose the setter of this property. @@ -42,50 +31,28 @@ Activity.Kind is already exposed property but we have exposed it as a read-only } ``` -## ActivityTagsCollection - -OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. The value can be numeric, bool, string types. Also, the value can be an array of one of these basic types. - -> ```json -> 'x-forwarded-for': 'foo.bar' -> 'timeout': 5 -> ``` +## ActivityAttributesCollection -There is OT proposal tracked by the [PR](https://github.com/open-telemetry/opentelemetry-specification/pull/596) to support the nested attributes. If this get accepted in OT, it will be easy to support it using ActivityTagsCollection. +OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. The value can be of different types (e.g. numeric, bool, string, arrays...etc.). > ```json -> 'http': { -> 'headers': [ -> 'x-forwarded-for': 'foo.bar' -> 'keep-alive': 'timeout=5' -> ] -> } +> 'x-forwarded-for': 'foo.bar' +> 'timeout': 5 > ``` -The attributes are used in multiple places in the OT (e.g. in Span, Event, and Link). Activity support Tags which similar concept but Tags is limited to string value types and not supporting any other types `IEnumerable> Tags`. We'll need to extend the Tags concept in our APIs to support the OT scenarios. We'll do that by introducing the ActivityTagsCollection and extending Activity class to support adding different types of tags value. - -ActivityTagsCollection is needed for the following reasons: +The attributes are used in multiple places in the OT (e.g. in Span, Event, and Link). Activity support Tags which similar concept but Tags is limited to string value types and not supporting any other types `IEnumerable> Tags`. We'll need to extend the Tags concept in our APIs to support the OT scenarios. We'll do that by introducing the ActivityAttributesCollection and extending Activity class to support adding different types of attribute values. -- We need to avoid having the consumers of our APIs to use any other types (e.g. Dictionary<>, List<>...etc.) which can cause issues like not keeping the items in order or not conforming to the OT specs which suggest specific behaviors (e.g. when adding null value, should remove the entry from the list) -- It is currently proposed to support nested attributes. ActivityTagsCollection will make it easy to do so at anytime is needed. +ActivityAttributesCollection is needed to ensure the OT specs behavior is implemented and avoid API users to use different type of collections (e.g. Dictionary<>, List<>...etc.) which not conforming with the OT specs. The OT specs require the attributes always stored in order and define behavior when adding values for keys which already existed in the list, it will override the old value. In addition it define the behavior what should happen when using null values. ```c# - public class ActivityTagsCollection : IReadOnlyCollection> + public class ActivityAttributesCollection : IReadOnlyCollection> { - public ActivityTagsCollection() { } - public ActivityTagsCollection(IEnumerable>) { } - - public void Add(string key, int value) { } - public void Add(string key, double value) { } - public void Add(string key, bool value) { } - public void Add(string key, string value) { } - public void Add(string key, int [] value) { } - public void Add(string key, double [] value) { } - public void Add(string key, bool [] value) { } - public void Add(string key, string [] value) { } + public ActivityAttributesCollection() { } + public ActivityAttributesCollection(IEnumerable>) { } - // The following API is pending the approval in OT spec first before we add it in .NET. - public void Add(string key, ActivityTagsCollection value) { } + // If the key was not set before, it will get added to the collections with the input value. + // If the key was set before, the mapped value of this key will be updated with the new input value. + public void Set(string key, int value) { } // interfaces implementation public int Count { get; } @@ -94,41 +61,26 @@ ActivityTagsCollection is needed for the following reasons: } ``` -## Activity.Tags +## Activity Attributes -As mentioned earlier, Activity has Tags property which returns the list of the activity tags. Tags are a list of `KeyValuePair`. Tags are proven to be not enough for OT scenarios as we need to support more value types (numeric, bool, numeric array, and nested tags). Activity today has the method `public Activity AddTag(string key, string? value)`. The proposal here is to add more `AddTag` overload accepting more types and add a new getter property return the list of all typed tags. +As mentioned earlier, Activity has Tags property which returns the list of the activity tags. Tags are a list of `KeyValuePair`. Tags are proven to be not enough for OT scenarios as we need to support more value types (numeric, bool, arrays...etc.). Activity today has the method `public Activity AddTag(string key, string? value)`. The proposal here is to add `SetAttribute()` which can set attributes value for a string key. + +In general, Attributes will be considered a superset of Tags. ```C# public partial class Activity : IDisposable { - public void AddTag(string key, int value) { } - public void AddTag(string key, double value) { } - public void AddTag(string key, bool value) { } - public void AddTag(string key, int [] value) { } - public void AddTag(string key, double [] value) { } - public void AddTag(string key, bool [] value) { } // how interesting this one? - public void AddTag(string key, string [] value) { } - - // The following API is pending the approval in OT spec first before we add it in .NET. - public void AddTag(string key, ActivityTagsCollection value) { } - - // We cannot change Tags property so we are introducing a new property with a new name - public ActivityTagsCollection TypedTags { get; } + public void SetAttribute(string key, object value) { } + + public ActivityAttributesCollection Attributes { get; } } ``` -***OpenQuestion*** - -We need to decide about the behavior of the old property `Activity.Tags` when adding tags of non-string types values. The options are - -- Ignore all non-string typed values and return only the string type value tags. -- Convert the non-string type values to string before returning it. - -I am inclining to the first option to avoid handling ToString with more complex types (e.g. arrays or ActivityTagsCollection). +`Activity.Tags` behavior is not going to change and will always return the list of tags/attributes which has string typed values only. while the new API `Activity.Attributes' is going to return all attributes with all different type values. ## Proposal Changing already exposed APIs in previous preview releases -As we are introducing the ActivityTagsCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. Also, we are renaming Attributes to Tags for consistency inside BCL APIs. +As we are introducing the ActivityAttributesCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. Also, we are renaming Attributes to Tags for consistency inside BCL APIs. ### ActivityEvent @@ -144,9 +96,9 @@ As we are introducing the ActivityTagsCollection, we can take advantage of that // change it to the following: public readonly struct ActivityEvent { - public ActivityEvent(string name, ActivityTagsCollection? tags) - public ActivityEvent(string name, DateTimeOffset timestamp, ActivityTagsCollection? tags) - public ActivityTagsCollection? Tags { get; } + public ActivityEvent(string name, ActivityAttributesCollection? tags) + public ActivityEvent(string name, DateTimeOffset timestamp, ActivityAttributesCollection? tags) + public ActivityAttributesCollection? Attributes { get; } } ``` @@ -163,11 +115,19 @@ As we are introducing the ActivityTagsCollection, we can take advantage of that // change it to the following: public readonly partial struct ActivityLink : IEquatable { - public ActivityLink(ActivityContext context, ActivityTagsCollection? tags) - public ActivityTagsCollection? Tags { get; } + public ActivityLink(ActivityContext context, ActivityAttributesCollection? tags) + public ActivityAttributesCollection? Attributes { get; } } ``` +## Handle ActivitySource.StartActivity(..., string parentId,....) case + +When API users call `ActivitySource.StartActivity(..., string parentId,....) `, we eventually call all listeners callback `ActivityListener.GetRequestedDataUsingParentId` and we pass the parent Id string to that callback. parent Id string can be a regular string or it can be constructed from the parent context (trace Id, span Id, and the trace flags). The feedback we got from OT is this design will require the implementers to try to extract the context from the parent Id. if the context cannot be extracted, the passed information will be kind not useful for the OT scenarios (the parent Id can be useful in other scenarios though). That is mean the burden will be on the listener implementer and will require them to implement `ActivityListener.GetRequestedDataUsingParentId` while not be used in some cases when cannot extract the context. + +The proposed change here is we'll have `ActivitySource.StartActivity(..., string parentId,....)` try to extract the context from the input parent Id. If it succeed to do that, it will call the listener callback `ActivityListener.GetRequestedDataUsingContex` instead and `ActivityListener.GetRequestedDataUsingParentId` will not get called. If it fail to construct the context, it will continue calling `ActivityListener.GetRequestedDataUsingParentId` if the listener implemented it. Listener implementers now have the choice to implement `ActivityListener.GetRequestedDataUsingParentId` only if they cares about parent Ids that don't include context information. + +There is no APIs change here as all design change is more on the internal implementation details. + ## Automatic Trace Id generation in case of null parent Sampler like probabilistic sampler can depend on the trace id for the sampling decision. Usually before creating any Activity using ActivitySource, the `ActivityListener.GetRequestedDataUsingContext` callback will get called which usually get implemented by the samplers. The parent context is sent as a parameter to this callback and the sampler gets the trace id from this parent context to perform the sampling action. When there is no parent, we send the default context which is nulling the trace id. The samplers depending on trace id are not going to be able to do the sampling at that time. The samplers even cannot generate and use a trace id because it cannot inform the Activity creation process to use the generated trace Id when creating the Activity object. From 9a55d1d2a0f86702e2c1ca31802949e9f7e1a674 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 24 Jun 2020 17:24:34 -0700 Subject: [PATCH 08/14] Update activity-improvements-2.md --- .../diagnostics/activity-improvements-2.md | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index b6902b0e6..7129612f3 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -80,7 +80,7 @@ In general, Attributes will be considered a superset of Tags. ## Proposal Changing already exposed APIs in previous preview releases -As we are introducing the ActivityAttributesCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. Also, we are renaming Attributes to Tags for consistency inside BCL APIs. +As we are introducing the ActivityAttributesCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. ### ActivityEvent @@ -96,8 +96,8 @@ As we are introducing the ActivityAttributesCollection, we can take advantage of // change it to the following: public readonly struct ActivityEvent { - public ActivityEvent(string name, ActivityAttributesCollection? tags) - public ActivityEvent(string name, DateTimeOffset timestamp, ActivityAttributesCollection? tags) + public ActivityEvent(string name, ActivityAttributesCollection? attributes) + public ActivityEvent(string name, DateTimeOffset timestamp, ActivityAttributesCollection? attributes) public ActivityAttributesCollection? Attributes { get; } } ``` @@ -115,19 +115,11 @@ As we are introducing the ActivityAttributesCollection, we can take advantage of // change it to the following: public readonly partial struct ActivityLink : IEquatable { - public ActivityLink(ActivityContext context, ActivityAttributesCollection? tags) + public ActivityLink(ActivityContext context, ActivityAttributesCollection? attributes) public ActivityAttributesCollection? Attributes { get; } } ``` -## Handle ActivitySource.StartActivity(..., string parentId,....) case - -When API users call `ActivitySource.StartActivity(..., string parentId,....) `, we eventually call all listeners callback `ActivityListener.GetRequestedDataUsingParentId` and we pass the parent Id string to that callback. parent Id string can be a regular string or it can be constructed from the parent context (trace Id, span Id, and the trace flags). The feedback we got from OT is this design will require the implementers to try to extract the context from the parent Id. if the context cannot be extracted, the passed information will be kind not useful for the OT scenarios (the parent Id can be useful in other scenarios though). That is mean the burden will be on the listener implementer and will require them to implement `ActivityListener.GetRequestedDataUsingParentId` while not be used in some cases when cannot extract the context. - -The proposed change here is we'll have `ActivitySource.StartActivity(..., string parentId,....)` try to extract the context from the input parent Id. If it succeed to do that, it will call the listener callback `ActivityListener.GetRequestedDataUsingContex` instead and `ActivityListener.GetRequestedDataUsingParentId` will not get called. If it fail to construct the context, it will continue calling `ActivityListener.GetRequestedDataUsingParentId` if the listener implemented it. Listener implementers now have the choice to implement `ActivityListener.GetRequestedDataUsingParentId` only if they cares about parent Ids that don't include context information. - -There is no APIs change here as all design change is more on the internal implementation details. - ## Automatic Trace Id generation in case of null parent Sampler like probabilistic sampler can depend on the trace id for the sampling decision. Usually before creating any Activity using ActivitySource, the `ActivityListener.GetRequestedDataUsingContext` callback will get called which usually get implemented by the samplers. The parent context is sent as a parameter to this callback and the sampler gets the trace id from this parent context to perform the sampling action. When there is no parent, we send the default context which is nulling the trace id. The samplers depending on trace id are not going to be able to do the sampling at that time. The samplers even cannot generate and use a trace id because it cannot inform the Activity creation process to use the generated trace Id when creating the Activity object. @@ -143,3 +135,10 @@ I am inclining to the first solution for the following reasons: - Exposing extra property can confuse the API consumers to know exactly when this property is needed or even they should care about it. - Adding such property to `ActivityCreationOptions` will increase the size of this struct which we are trying to make it compact for perf reasons. - I am not seeing a strong reason why the first solution wouldn't be good enough. + +## Handle ActivitySource.StartActivity(..., string parentId,....) case + +When API users call `ActivitySource.StartActivity(..., string parentId,....) `, we eventually call all listeners callback `ActivityListener.GetRequestedDataUsingParentId` and we pass the parent Id string to that callback. parent Id string can be a regular string or it can be constructed from the parent context (trace Id, span Id, and the trace flags). The feedback we got from OT is this design will require the implementers to try to extract the context from the parent Id. If the context cannot be extracted, the passed information will be kind not useful for the OT scenarios (the parent Id can be useful in other scenarios though). That is mean the burden will be on the listener implementer and will require them to implement `ActivityListener.GetRequestedDataUsingParentId` while not be used in some cases when cannot extract the context. + +The proposed change here is when calling `ActivitySource.StartActivity(..., string parentId,....)` and the listener implementing the callback `ActivityListener.GetRequestedDataUsingParentId`, we'll call this callback as we do today as the listener showing the intend to receive and handle the Parent Id. If the listener didn't implement `ActivityListener.GetRequestedDataUsingParentId` and implementing `ActivityListener.GetRequestedDataUsingContex` then we'll try convert the parent Id to context. If the conversion succeed well call `ActivityListener.GetRequestedDataUsingContex` otherwise we'll not call any listeners callbacks. +There is no APIs change here as all design change is more on the internal implementation details. From 5237ba744bb4c3bd9f0d149e745439833e1b1c28 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 25 Jun 2020 11:40:12 -0700 Subject: [PATCH 09/14] Update activity-improvements-2.md --- accepted/2020/diagnostics/activity-improvements-2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index 7129612f3..d3d6e9edb 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -52,7 +52,7 @@ ActivityAttributesCollection is needed to ensure the OT specs behavior is implem // If the key was not set before, it will get added to the collections with the input value. // If the key was set before, the mapped value of this key will be updated with the new input value. - public void Set(string key, int value) { } + public void Set(string key, object value) { } // interfaces implementation public int Count { get; } From 0a426fda67e2cb131ec5f4bc8d6ff2f67cc8060b Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 25 Jun 2020 18:49:30 -0700 Subject: [PATCH 10/14] Update activity-improvements-2.md --- .../diagnostics/activity-improvements-2.md | 78 ++++++++++--------- 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index d3d6e9edb..7391c9d0f 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -1,6 +1,6 @@ # Improve Activity API usability and OpenTelemetry integration (Part 2) -We have exposed some [Activity APIs to improve the API usability and to integrate with OpenTelemetry](https://github.com/dotnet/designs/pull/98) (OT) so Activity can be a suitable replacement for the OT Span type described by the [OT Specs](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span). We have received feedback regarding some OT blocked scenarios which need to be addressed in the Activity APIs and unblock the OT SDK implementation. +We have exposed [Activity APIs to improve the API usability and to integrate with OpenTelemetry](https://github.com/dotnet/designs/pull/98) (OT) to have `Activity` be suitable replacement for the OT Span type described by the [OT Specs](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span). We have received feedback regarding some OT blocked scenarios which need to be addressed in the Activity APIs and unblock the OT SDK implementation. ## ActivityContext.IsRemote @@ -31,18 +31,43 @@ Activity.Kind is already exposed property but we have exposed it as a read-only } ``` -## ActivityAttributesCollection +## Activity Attributes OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. The value can be of different types (e.g. numeric, bool, string, arrays...etc.). > ```json -> 'x-forwarded-for': 'foo.bar' -> 'timeout': 5 +> 'x-forwarded-for': 'foo.bar' +> 'timeout': 5 > ``` -The attributes are used in multiple places in the OT (e.g. in Span, Event, and Link). Activity support Tags which similar concept but Tags is limited to string value types and not supporting any other types `IEnumerable> Tags`. We'll need to extend the Tags concept in our APIs to support the OT scenarios. We'll do that by introducing the ActivityAttributesCollection and extending Activity class to support adding different types of attribute values. +The attributes are used in multiple places in the OT (e.g. in Span, Event, and Link). Activity support Tags which similar concept but Tags is limited to string value types and not supporting any other types `IEnumerable> Tags`. Tags are proven to be not enough for OT scenarios as we need to support more value types (numeric, bool, arrays...etc.). Activity today has the method `public Activity AddTag(string key, string? value)`. The proposal here is to add `SetAttribute()` which can set attributes value for a string key. + +In general, Attributes will be considered a superset of Tags. + +```C# + public partial class Activity : IDisposable + { + public void SetAttribute(string key, object value) { } + + public IEnumerator> Attributes { get; } + } +``` + +`Activity.Tags` behavior is not going to change and will always return the list of tags/attributes which has string typed values only. while the new API `Activity.Attributes' is going to return all attributes with all different type values. + +## ActivityAttributesCollection + +As discussed in previous section OT has the concept of `Attributes` which we proposed supporting it in `Activity` class. `Attributes` is used in some other types in OT too, `ActivityEvent` and `ActivityLink`. The users of these types will need to create the Attributes collection and pass it to the type constructors. + +OT define some specific behavior for the `Attributes` collection: + +- The collection items has to be ordered as it get added. +- Adding item which has a key equal to a previously added item's key, will update the value of the existing item. in other word, the collections shouldn't include 2 items with same key. +- Adding item with `null` value will delete the item which has key equal to the input item's key. + +Obviously if we didn't provide a collection behaving as the OT spec require, every user of our APIs will need to implement this behavior manually. Or will use other collection (e.g. `Dictionary` or `List`) and at that time the behavior will be wrong and we can run into problems when not matching OT specs. -ActivityAttributesCollection is needed to ensure the OT specs behavior is implemented and avoid API users to use different type of collections (e.g. Dictionary<>, List<>...etc.) which not conforming with the OT specs. The OT specs require the attributes always stored in order and define behavior when adding values for keys which already existed in the list, it will override the old value. In addition it define the behavior what should happen when using null values. +The proposal here is to provide ActivityAttributesCollection which will help ensuring the OT behavior and make it easy for the users to handle the `Attributes` in general. ```c# public class ActivityAttributesCollection : IReadOnlyCollection> @@ -61,26 +86,9 @@ ActivityAttributesCollection is needed to ensure the OT specs behavior is implem } ``` -## Activity Attributes - -As mentioned earlier, Activity has Tags property which returns the list of the activity tags. Tags are a list of `KeyValuePair`. Tags are proven to be not enough for OT scenarios as we need to support more value types (numeric, bool, arrays...etc.). Activity today has the method `public Activity AddTag(string key, string? value)`. The proposal here is to add `SetAttribute()` which can set attributes value for a string key. - -In general, Attributes will be considered a superset of Tags. - -```C# - public partial class Activity : IDisposable - { - public void SetAttribute(string key, object value) { } - - public ActivityAttributesCollection Attributes { get; } - } -``` - -`Activity.Tags` behavior is not going to change and will always return the list of tags/attributes which has string typed values only. while the new API `Activity.Attributes' is going to return all attributes with all different type values. - ## Proposal Changing already exposed APIs in previous preview releases -As we are introducing the ActivityAttributesCollection, we can take advantage of that now and modify some of the exposed APIs to use this collection instead. +As we are introducing the ActivityAttributesCollection, we can now modify some of the exposed APIs to use this collection instead. ### ActivityEvent @@ -90,7 +98,6 @@ As we are introducing the ActivityAttributesCollection, we can take advantage of { public ActivityEvent(string name, IEnumerable>? attributes) public ActivityEvent(string name, DateTimeOffset timestamp, IEnumerable>? attributes) - public IEnumerable> Attributes { get; } } // change it to the following: @@ -98,7 +105,6 @@ As we are introducing the ActivityAttributesCollection, we can take advantage of { public ActivityEvent(string name, ActivityAttributesCollection? attributes) public ActivityEvent(string name, DateTimeOffset timestamp, ActivityAttributesCollection? attributes) - public ActivityAttributesCollection? Attributes { get; } } ``` @@ -109,14 +115,12 @@ As we are introducing the ActivityAttributesCollection, we can take advantage of public readonly partial struct ActivityLink : IEquatable { public ActivityLink(ActivityContext context, IEnumerable>? attributes) - public IEnumerable>? Attributes { get; } } // change it to the following: public readonly partial struct ActivityLink : IEquatable { public ActivityLink(ActivityContext context, ActivityAttributesCollection? attributes) - public ActivityAttributesCollection? Attributes { get; } } ``` @@ -125,19 +129,23 @@ As we are introducing the ActivityAttributesCollection, we can take advantage of Sampler like probabilistic sampler can depend on the trace id for the sampling decision. Usually before creating any Activity using ActivitySource, the `ActivityListener.GetRequestedDataUsingContext` callback will get called which usually get implemented by the samplers. The parent context is sent as a parameter to this callback and the sampler gets the trace id from this parent context to perform the sampling action. When there is no parent, we send the default context which is nulling the trace id. The samplers depending on trace id are not going to be able to do the sampling at that time. The samplers even cannot generate and use a trace id because it cannot inform the Activity creation process to use the generated trace Id when creating the Activity object. There are a couple of proposals how to address this issue in the .NET runtime. The idea is the runtime need to generate the trace Id and send it to the samplers. If the sampler using the passed trace id and decide to sample in this case, the Activity which will get created because of that sampler decision will be using the generated trace Id in its context. -Solution options: +The proposal here is to expose the `ActivityListener` property -- Before calling `ActivityListener.GetRequestedDataUsingContext`, there will be a check if we don't have a parent, then we generate a random trace Id and use it in the context sent to the callback. If decided to sample in, we all use this trace Id during the Activity object creation. This option doesn't need any API changes and will be fully handled inside the implementation. -- Add a new property to the `ActivityCreationOptions` struct (e.g. `NullParentTraceId`) which will carry the trace id that can be used by the sampler when detecting no parent context. obviously this will need API change. +```c# + public sealed class ActivityListener : IDisposable // already existing type + { + public bool PregenerateNewRootId { get; set;} // other name suggestion: GenerateTraceIdForRootContext + } +``` -I am inclining to the first solution for the following reasons: +Before calling the listener, if we have a null parent context and this new property is set to true, we'll generate a trace Id and set it in the parent context send it to the listener. later if decided to create a new `Activity` object as a result of listener response, we'll use the generated trace id with the new created `Activity` object. -- Exposing extra property can confuse the API consumers to know exactly when this property is needed or even they should care about it. -- Adding such property to `ActivityCreationOptions` will increase the size of this struct which we are trying to make it compact for perf reasons. -- I am not seeing a strong reason why the first solution wouldn't be good enough. +We are making this as opt-in option in the listener to avoid any performance issue with the default scenarios which not caring about getting the parent trace Id when having null parent. Generating the trace id will require some memory allocation. ## Handle ActivitySource.StartActivity(..., string parentId,....) case +***This is implementation details and no APIs change. can safely ignored by the design reviewer*** + When API users call `ActivitySource.StartActivity(..., string parentId,....) `, we eventually call all listeners callback `ActivityListener.GetRequestedDataUsingParentId` and we pass the parent Id string to that callback. parent Id string can be a regular string or it can be constructed from the parent context (trace Id, span Id, and the trace flags). The feedback we got from OT is this design will require the implementers to try to extract the context from the parent Id. If the context cannot be extracted, the passed information will be kind not useful for the OT scenarios (the parent Id can be useful in other scenarios though). That is mean the burden will be on the listener implementer and will require them to implement `ActivityListener.GetRequestedDataUsingParentId` while not be used in some cases when cannot extract the context. The proposed change here is when calling `ActivitySource.StartActivity(..., string parentId,....)` and the listener implementing the callback `ActivityListener.GetRequestedDataUsingParentId`, we'll call this callback as we do today as the listener showing the intend to receive and handle the Parent Id. If the listener didn't implement `ActivityListener.GetRequestedDataUsingParentId` and implementing `ActivityListener.GetRequestedDataUsingContex` then we'll try convert the parent Id to context. If the conversion succeed well call `ActivityListener.GetRequestedDataUsingContex` otherwise we'll not call any listeners callbacks. From 98b7a67345f81a3bd992e2953eb349c1fe02b1a4 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 26 Jun 2020 09:33:50 -0700 Subject: [PATCH 11/14] Update activity-improvements-2.md --- accepted/2020/diagnostics/activity-improvements-2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index 7391c9d0f..841056370 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -134,7 +134,7 @@ The proposal here is to expose the `ActivityListener` property ```c# public sealed class ActivityListener : IDisposable // already existing type { - public bool PregenerateNewRootId { get; set;} // other name suggestion: GenerateTraceIdForRootContext + public bool PregenerateNewRootId { get; set;} // other names suggestion: GenerateTraceIdForRootContext, GenerateTraceIdForNullParent } ``` From d0c5de25bd54548317969f4e39e826f336a54c8f Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 1 Jul 2020 12:46:42 -0700 Subject: [PATCH 12/14] Update activity-improvements-2.md --- accepted/2020/diagnostics/activity-improvements-2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index 841056370..715c2cad5 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -73,7 +73,7 @@ The proposal here is to provide ActivityAttributesCollection which will help ens public class ActivityAttributesCollection : IReadOnlyCollection> { public ActivityAttributesCollection() { } - public ActivityAttributesCollection(IEnumerable>) { } + public ActivityAttributesCollection(IEnumerable> attributes) { } // If the key was not set before, it will get added to the collections with the input value. // If the key was set before, the mapped value of this key will be updated with the new input value. From 9ad285b3e3382412beb53b612a1fe7220747fac2 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 15 Jul 2020 15:09:27 -0700 Subject: [PATCH 13/14] Update activity-improvements-2.md --- .../diagnostics/activity-improvements-2.md | 127 +++++++++++++++++- 1 file changed, 124 insertions(+), 3 deletions(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index 715c2cad5..a2f908f75 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -2,12 +2,16 @@ We have exposed [Activity APIs to improve the API usability and to integrate with OpenTelemetry](https://github.com/dotnet/designs/pull/98) (OT) to have `Activity` be suitable replacement for the OT Span type described by the [OT Specs](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span). We have received feedback regarding some OT blocked scenarios which need to be addressed in the Activity APIs and unblock the OT SDK implementation. +[Design Review Notes and Video](https://github.com/dotnet/runtime/issues/38419#issuecomment-655054487) + ## ActivityContext.IsRemote We already exposed the ActivityContext type as described by the OT specs. There is a property inside this type called `IsRemote` which we didn't include in the first place because it was under discussion with the OT spec committee. It has been confirmed this property is needed and will stay as part of the OT specs. [ActivityContext.IsRemote](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#spancontext) is a Boolean flag which returns true if the ActivityContext was propagated from a remote parent. When creating children from remote spans, their IsRemote flag MUST be set to false. +## ***API Proposal*** + ```c# public readonly partial struct ActivityContext : IEquatable { @@ -19,10 +23,16 @@ We already exposed the ActivityContext type as described by the OT specs. There } ``` +## ***Design Review Decision*** + +The Proposal has been accepted without any change. + ## Activity.Kind Setter Activity.Kind is already exposed property but we have exposed it as a read-only property. We got OT adapters scenario that listen to the old created activities (by asp.net and Http clients). As the old Activities didn't support Kind property, the adapter needs to set this property before sending the activity to the sampling in the OT SDK. The proposal here is to expose the setter of this property. +## ***API Proposal*** + ```c# public partial class Activity : IDisposable { @@ -31,19 +41,25 @@ Activity.Kind is already exposed property but we have exposed it as a read-only } ``` +## ***Design Review Decision*** + +This API has been rejected because making `Activity.Kind` settable seems very unfortunate. Given we do this backward compatibility only, we should find another way of doing this without making one of the key properties mutable. One option is to disallow changing after it was observed (whatever that means). The preferred option would be to find a way that doesn't require mutation (constructor parameter, factory method etc.). This part requires more design. + ## Activity Attributes OT has the concept of [Attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes) which is a list of key-value pairs mapping a string to some value. The value can be of different types (e.g. numeric, bool, string, arrays...etc.). > ```json -> 'x-forwarded-for': 'foo.bar' -> 'timeout': 5 +> 'x-forwarded-for': 'foo.bar' +> 'timeout': 5 > ``` The attributes are used in multiple places in the OT (e.g. in Span, Event, and Link). Activity support Tags which similar concept but Tags is limited to string value types and not supporting any other types `IEnumerable> Tags`. Tags are proven to be not enough for OT scenarios as we need to support more value types (numeric, bool, arrays...etc.). Activity today has the method `public Activity AddTag(string key, string? value)`. The proposal here is to add `SetAttribute()` which can set attributes value for a string key. In general, Attributes will be considered a superset of Tags. +## ***API Proposal*** + ```C# public partial class Activity : IDisposable { @@ -55,6 +71,35 @@ In general, Attributes will be considered a superset of Tags. `Activity.Tags` behavior is not going to change and will always return the list of tags/attributes which has string typed values only. while the new API `Activity.Attributes' is going to return all attributes with all different type values. +## ***Design Review Decision*** + +It has been decided will be confusing if we introduce a new concept with Attributes especially there is already confusion between `Tags` and `Baggages`. so the decision is to continue using `Tag` name instead of `Attribute`. Also, it is decided to provide the API `AddTag(string, object)` which will behave like the other existing overload `AddTag(string, string)`. That means, `AddTag` will allow adding tags with same keys and will allow adding tags with `null` values. + +`SetTag(string, object)` will be added too but will behave according to OpenTelemetry specs which is + +If the input value is null + +- if the collection has any tag with the same key, then this tag will get removed from the collection. + + - otherwise, nothing will happen and the collection will not change. + +If the input value is not null + + - if the collection has any tag with the same key, then the value mapped to this key will get updated with the new input value. + + - otherwise, the key and value will get added as a new tag to the collection. + +Finally, we are going to have the property `TagObjects` to return the whole set of tags. `TagObject` which has object values can be considered a superset of `Tags` which has string values only. + +```C# + public partial class Activity + { + public void AddTag(string key, object value); + public void SetTag(string key, object value); + public IEnumerable> TagObjects { get; } + } +``` + ## ActivityAttributesCollection As discussed in previous section OT has the concept of `Attributes` which we proposed supporting it in `Activity` class. `Attributes` is used in some other types in OT too, `ActivityEvent` and `ActivityLink`. The users of these types will need to create the Attributes collection and pass it to the type constructors. @@ -69,6 +114,8 @@ Obviously if we didn't provide a collection behaving as the OT spec require, eve The proposal here is to provide ActivityAttributesCollection which will help ensuring the OT behavior and make it easy for the users to handle the `Attributes` in general. +## ***API Proposal*** + ```c# public class ActivityAttributesCollection : IReadOnlyCollection> { @@ -86,10 +133,52 @@ The proposal here is to provide ActivityAttributesCollection which will help ens } ``` -## Proposal Changing already exposed APIs in previous preview releases +## ***Design Review Decision*** + +It is decided to rename it to `ActivityTagsCollection` and to Implement `IDictionary` interface instead of `IReadOnlyCollection`. + +```C# + public class ActivityTagsCollection : IDictionary + { + public ActivityTagsCollection() { throw null; } + public ActivityTagsCollection(IEnumerable> list); + public object? this[string key] { get; set; } + public ICollection Keys { get; } + public ICollection Values { get; } + public int Count { get; } + public bool IsReadOnly { get; } + public void Add(string key, object value); + public void Add(KeyValuePair item); + public void Clear(); + public bool Contains(KeyValuePair item); + public bool ContainsKey(string key); + public void CopyTo(KeyValuePair[] array, int arrayIndex); + IEnumerator> IEnumerable>.GetEnumerator(); + public bool Remove(string key); + public bool Remove(KeyValuePair item); + public bool TryGetValue(string key, out object value); + IEnumerator IEnumerable.GetEnumerator(); + public Enumerator GetEnumerator(); + + public struct Enumerator : IEnumerator>, IEnumerator + { + public KeyValuePair Current { get; } + object IEnumerator.Current { get; } + public void Dispose(); + public bool MoveNext(); + void IEnumerator.Reset(); + } + } +``` + + + +## Proposal: Changing already exposed APIs in previous preview releases As we are introducing the ActivityAttributesCollection, we can now modify some of the exposed APIs to use this collection instead. +## ***API Proposal*** + ### ActivityEvent ```C# @@ -124,6 +213,23 @@ As we are introducing the ActivityAttributesCollection, we can now modify some o } ``` +## ***Design Review Decision*** + +Mainly the proposal accepted with small tweak. `ActivityEvent` and `ActivityLink` will now include only the following constructors: + +```C# + public readonly struct ActivityEvent + { + public ActivityEvent(string name); + public ActivityEvent(string name, System.DateTimeOffset timestamp = default, ActivityTagsCollection? tags = null); + } + + public readonly struct ActivityLink : IEquatable + { + public ActivityLink(ActivityContext context, ActivityTagsCollection? tags = null); + } +``` + ## Automatic Trace Id generation in case of null parent Sampler like probabilistic sampler can depend on the trace id for the sampling decision. Usually before creating any Activity using ActivitySource, the `ActivityListener.GetRequestedDataUsingContext` callback will get called which usually get implemented by the samplers. The parent context is sent as a parameter to this callback and the sampler gets the trace id from this parent context to perform the sampling action. When there is no parent, we send the default context which is nulling the trace id. The samplers depending on trace id are not going to be able to do the sampling at that time. The samplers even cannot generate and use a trace id because it cannot inform the Activity creation process to use the generated trace Id when creating the Activity object. @@ -131,6 +237,8 @@ There are a couple of proposals how to address this issue in the .NET runtime. T The proposal here is to expose the `ActivityListener` property +## ***API Proposal*** + ```c# public sealed class ActivityListener : IDisposable // already existing type { @@ -142,6 +250,19 @@ Before calling the listener, if we have a null parent context and this new prope We are making this as opt-in option in the listener to avoid any performance issue with the default scenarios which not caring about getting the parent trace Id when having null parent. Generating the trace id will require some memory allocation. +## ***Design Review Decision*** + +This API is accepted but it is renamed to `AutoGenerateRootContextTraceId ` + +```C# + public sealed class ActivityListener + { + public bool AutoGenerateRootContextTraceId { get; set; } + } +``` + + + ## Handle ActivitySource.StartActivity(..., string parentId,....) case ***This is implementation details and no APIs change. can safely ignored by the design reviewer*** From 32b2b237c58c60d3030a511acfdf041b25a14f6e Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 15 Jul 2020 15:14:22 -0700 Subject: [PATCH 14/14] Update activity-improvements-2.md --- accepted/2020/diagnostics/activity-improvements-2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accepted/2020/diagnostics/activity-improvements-2.md b/accepted/2020/diagnostics/activity-improvements-2.md index a2f908f75..0f232530d 100644 --- a/accepted/2020/diagnostics/activity-improvements-2.md +++ b/accepted/2020/diagnostics/activity-improvements-2.md @@ -252,7 +252,7 @@ We are making this as opt-in option in the listener to avoid any performance iss ## ***Design Review Decision*** -This API is accepted but it is renamed to `AutoGenerateRootContextTraceId ` +This API is accepted but the property renamed to `AutoGenerateRootContextTraceId ` ```C# public sealed class ActivityListener