From 3e8ca3d2d73c82bc5d44e1f2b42779ae01b04c89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 4 Oct 2019 14:42:15 +0200 Subject: [PATCH 01/13] Add new OTEP: unseal-spancontext --- text/0000-unseal-spancontext.md | 85 +++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 text/0000-unseal-spancontext.md diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md new file mode 100644 index 000000000..b185471c4 --- /dev/null +++ b/text/0000-unseal-spancontext.md @@ -0,0 +1,85 @@ +# Opening Span's Context + +**Status:** `proposed` + +Remove the proposition that "`SpanContext` MUST be a final (sealed) class.". + +## Motivation + +Currently, SpanContext is the only class that has such a limitation placed upon it in the Spec. +I also haven't heard a good argument for having a final `SpanContext` so this seems an arbitrary restriction. + +On the other hand, a non-final SpanContext does have advantages for vendor's flexibility. For example, they could: + +* Add custom routing information to the SpanContext more efficiently than by using the string -> string mapping in TraceState. +* Generate span IDs lazily (e.g. if a Span only has internal parents and children, a mechanism that is more efficient than a GUID could be used) +* Count the number of times a span context was injected to better estimate on the back-end how long to wait for (additional) child spans. + +## Explanation + +Spans remain as a non-abstract class implemented in the API, but they are non-final. Note that the proposition + +> SpanContexts are immutable. + +only applies to the part of the SpanContext that is publicly exposed on the API level. +Actual vendor implementations could have additional mutable properties or +actually have the first access to the `TraceId` property + +## Internal details + +The changes in the initial summary of this OTEP shall be applied to the specification at +https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#spancontext. + +Language libraries that actually have a "final" or "sealed" keyword must remove it from the API definition of SpanContext. +The reference SDK implementation will continue to use this Span implementation without deriving from it. +Additionally, all public APIs of the SpanContext must be overridable +(e.g. they must not be public data fields in languages where these are not overridable and +languages that require an explicit marker like `virtual` to make an API overridable must add it). + +Existing propagators (which should be implemented in the SDK package or a separate package) can continue to directly construct a SpanContext. +API implementations that want to use a custom SpanContext implementation must either +handle having a plain SpanContext as parent +and having additional properties stripped upon extracting +or they provide their own propagator implementations too. + +## Trade-offs and mitigations + +The decision to have SpanContext as a non-abstract class in the API results from the goal to keep compatibility. +The cleaner alternative would be to have SpanContext be an interface like all other API classes. + +Since the implementation of propagators is on the SDK level, all public APIs could be removed from the SpanContext +so that it becomes an object that is completely opaque (to everyone but the propagator implementations). That would +enable even more flexibility or at least spare vendors from adding boilerplate code that e.g. calculates TraceIds +if there is no such concept in their own system. + +## Prior art and alternatives + +The topic was briefly discussed in +https://github.com/open-telemetry/opentelemetry-specification/pull/244 +""Fix" contradictory wording in SpanContext spec.". Before that PR the specification contained the following sentence that directly contradicted the requirement that a SpanContext should be final (that requirement was there too): + +> `SpanContext` is represented as an interface, in order to be serializable into a wider variety of trace context wire formats. + +In OpenCensus, the SpanContext was just an interface with very few public methods +(in particular, it did not assume the existence of a thing such as Trace or Span ID): + +> The `SpanContext` is more of a "concept" than a useful piece of functionality at the generic OpenTracing layer. That said, it is of critical importance to OpenTracing *implementations* and does present a thin API of its own. Most OpenTracing users only interact with `SpanContext` via [**references**](https://github.com/opentracing/specification/blob/master/specification.md#references-between-spans) when starting new `Span`s, or when injecting/extracting a trace to/from some transport protocol. + +(quoted from https://github.com/opentracing/specification/blob/master/specification.md#spancontext) + +In principle, the same still applies to OpenTelemetry: +SpanContext has no functionality that is useful outside of propagation or implementations of Spans and Tracers. However, we chose to expose the "internal details" of the W3C context in the API. + +## Open questions + +None known. + +## Future possibilities + +It is not expected that the OpenTelemetry reference implementation of the SDK or API benefits much from this change. +The reference SDK is blessed here +because if a new functionality is required in SpanContext (e.g., [IsRemote][]) +it can "just" be added to the API implementation. +However, vendors who e.g. want to store attributes that are only useful for their special implementation in the SpanContext can now do that. + +[IsRemote]: https://github.com/open-telemetry/opentelemetry-specification/pull/216 From 6e01d0d5704cceafbdb79f73ed4a7ffbc9ee9358 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 4 Oct 2019 14:46:01 +0200 Subject: [PATCH 02/13] Update to use new template as base. --- text/0000-unseal-spancontext.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index b185471c4..233446da7 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -1,7 +1,5 @@ # Opening Span's Context -**Status:** `proposed` - Remove the proposition that "`SpanContext` MUST be a final (sealed) class.". ## Motivation From 6aa86447cc6ca68f811121a18098f7d5fdbd77d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 4 Oct 2019 14:47:40 +0200 Subject: [PATCH 03/13] Fix incomplete sentence. --- text/0000-unseal-spancontext.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index 233446da7..c2ba8ae6f 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -21,7 +21,7 @@ Spans remain as a non-abstract class implemented in the API, but they are non-fi only applies to the part of the SpanContext that is publicly exposed on the API level. Actual vendor implementations could have additional mutable properties or -actually have the first access to the `TraceId` property +actually have the first access to the `TraceId` property generate the TraceId lazily. ## Internal details From 8efc035665e058ad3c8d79e10c8d73a34986390e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 4 Oct 2019 15:09:43 +0200 Subject: [PATCH 04/13] Incorporate review feedback. --- text/0000-unseal-spancontext.md | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index c2ba8ae6f..9bdd93bf9 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -4,14 +4,14 @@ Remove the proposition that "`SpanContext` MUST be a final (sealed) class.". ## Motivation -Currently, SpanContext is the only class that has such a limitation placed upon it in the Spec. +Currently, SpanContext is the only class that has such a limitation placed upon it in the specification. I also haven't heard a good argument for having a final `SpanContext` so this seems an arbitrary restriction. On the other hand, a non-final SpanContext does have advantages for vendor's flexibility. For example, they could: -* Add custom routing information to the SpanContext more efficiently than by using the string -> string mapping in TraceState. -* Generate span IDs lazily (e.g. if a Span only has internal parents and children, a mechanism that is more efficient than a GUID could be used) -* Count the number of times a span context was injected to better estimate on the back-end how long to wait for (additional) child spans. +* Add custom routing information to the SpanContext more efficiently than by using the string-based mapping in TraceState. +* Generate Span IDs lazily (e.g., if a Span only has internal parents and children, a mechanism that is more efficient than a combined 128 + 64 bit identifier could be used). +* Count the number of times a SpanContext was injected to better estimate on the back end how long to wait for (additional) child Spans. ## Explanation @@ -31,8 +31,8 @@ https://github.com/open-telemetry/opentelemetry-specification/blob/master/specif Language libraries that actually have a "final" or "sealed" keyword must remove it from the API definition of SpanContext. The reference SDK implementation will continue to use this Span implementation without deriving from it. Additionally, all public APIs of the SpanContext must be overridable -(e.g. they must not be public data fields in languages where these are not overridable and -languages that require an explicit marker like `virtual` to make an API overridable must add it). +(they must not be public data fields in languages where these are not overridable, +languages that require an explicit marker like `virtual` to make an API overridable must add it, etc.). Existing propagators (which should be implemented in the SDK package or a separate package) can continue to directly construct a SpanContext. API implementations that want to use a custom SpanContext implementation must either @@ -46,19 +46,19 @@ The decision to have SpanContext as a non-abstract class in the API results from The cleaner alternative would be to have SpanContext be an interface like all other API classes. Since the implementation of propagators is on the SDK level, all public APIs could be removed from the SpanContext -so that it becomes an object that is completely opaque (to everyone but the propagator implementations). That would -enable even more flexibility or at least spare vendors from adding boilerplate code that e.g. calculates TraceIds +so that it becomes an object that is completely opaque (to everyone but the propagator implementations). +This would enable even more flexibility or at least spare vendors from adding boilerplate code that, e.g., calculates TraceIds if there is no such concept in their own system. ## Prior art and alternatives The topic was briefly discussed in https://github.com/open-telemetry/opentelemetry-specification/pull/244 -""Fix" contradictory wording in SpanContext spec.". Before that PR the specification contained the following sentence that directly contradicted the requirement that a SpanContext should be final (that requirement was there too): +("Fix" contradictory wording in SpanContext spec). Before this PR the specification contained the following sentence that directly contradicted the requirement that a SpanContext should be final (that requirement was there too): > `SpanContext` is represented as an interface, in order to be serializable into a wider variety of trace context wire formats. -In OpenCensus, the SpanContext was just an interface with very few public methods +In OpenTelemetry, the SpanContext was just an interface with very few public methods (in particular, it did not assume the existence of a thing such as Trace or Span ID): > The `SpanContext` is more of a "concept" than a useful piece of functionality at the generic OpenTracing layer. That said, it is of critical importance to OpenTracing *implementations* and does present a thin API of its own. Most OpenTracing users only interact with `SpanContext` via [**references**](https://github.com/opentracing/specification/blob/master/specification.md#references-between-spans) when starting new `Span`s, or when injecting/extracting a trace to/from some transport protocol. @@ -70,7 +70,7 @@ SpanContext has no functionality that is useful outside of propagation or implem ## Open questions -None known. +None known yet. ## Future possibilities @@ -78,6 +78,6 @@ It is not expected that the OpenTelemetry reference implementation of the SDK or The reference SDK is blessed here because if a new functionality is required in SpanContext (e.g., [IsRemote][]) it can "just" be added to the API implementation. -However, vendors who e.g. want to store attributes that are only useful for their special implementation in the SpanContext can now do that. +But for example, vendors who want to store attributes that are only useful for their special implementation in the SpanContext can now do that. [IsRemote]: https://github.com/open-telemetry/opentelemetry-specification/pull/216 From 85f03f10b9c3e2322c0f2e27cd2af6d33d183754 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 4 Oct 2019 15:12:34 +0200 Subject: [PATCH 05/13] Wording. --- text/0000-unseal-spancontext.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index 9bdd93bf9..df40c7211 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -4,10 +4,10 @@ Remove the proposition that "`SpanContext` MUST be a final (sealed) class.". ## Motivation -Currently, SpanContext is the only class that has such a limitation placed upon it in the specification. +Currently, SpanContext is the only class in the specification that has the limitation of being final placed upon it. I also haven't heard a good argument for having a final `SpanContext` so this seems an arbitrary restriction. -On the other hand, a non-final SpanContext does have advantages for vendor's flexibility. For example, they could: +On the other hand, a non-final SpanContext does have advantages for vendors' flexibility. For example, they could: * Add custom routing information to the SpanContext more efficiently than by using the string-based mapping in TraceState. * Generate Span IDs lazily (e.g., if a Span only has internal parents and children, a mechanism that is more efficient than a combined 128 + 64 bit identifier could be used). @@ -54,7 +54,7 @@ if there is no such concept in their own system. The topic was briefly discussed in https://github.com/open-telemetry/opentelemetry-specification/pull/244 -("Fix" contradictory wording in SpanContext spec). Before this PR the specification contained the following sentence that directly contradicted the requirement that a SpanContext should be final (that requirement was there too): +("Fix" contradictory wording in SpanContext spec). Before this PR, the specification contained the following sentence that directly contradicted the requirement that a SpanContext should be final (that requirement was there too): > `SpanContext` is represented as an interface, in order to be serializable into a wider variety of trace context wire formats. From a45f3a115e4ec21c5c3b04f1a918bed33ce72eaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 4 Oct 2019 15:15:37 +0200 Subject: [PATCH 06/13] Add heads-up for mutability, typo. --- text/0000-unseal-spancontext.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index df40c7211..838c99890 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -15,13 +15,13 @@ On the other hand, a non-final SpanContext does have advantages for vendors' fle ## Explanation -Spans remain as a non-abstract class implemented in the API, but they are non-final. Note that the proposition +SpanContext remains as a non-abstract class implemented in the API, but they are non-final. Note that the proposition > SpanContexts are immutable. only applies to the part of the SpanContext that is publicly exposed on the API level. Actual vendor implementations could have additional mutable properties or -actually have the first access to the `TraceId` property generate the TraceId lazily. +actually have the first access to the `TraceId` property generate the TraceId lazily. Vendors must take care not to break any assumptions by doing that. For example, if a language allows copying a SpanContext, vendors should better put all mutable data behind an immutable reference so that all copies remain in a consistent state. ## Internal details From b3201a272188d556e69da6cb55db8f200c46233f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 4 Oct 2019 15:41:04 +0200 Subject: [PATCH 07/13] Add a few known issues with Mitigations. --- text/0000-unseal-spancontext.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index 838c99890..bfd5ffcc6 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -28,8 +28,9 @@ actually have the first access to the `TraceId` property generate the TraceId la The changes in the initial summary of this OTEP shall be applied to the specification at https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#spancontext. -Language libraries that actually have a "final" or "sealed" keyword must remove it from the API definition of SpanContext. -The reference SDK implementation will continue to use this Span implementation without deriving from it. +Language libraries that used a "final" or "sealed" keyword or something similar in their API definition of SpanContext must remove it. + +The reference SDK implementation will continue to use the API's SpanContext implementation without deriving from it. Additionally, all public APIs of the SpanContext must be overridable (they must not be public data fields in languages where these are not overridable, languages that require an explicit marker like `virtual` to make an API overridable must add it, etc.). @@ -50,6 +51,14 @@ so that it becomes an object that is completely opaque (to everyone but the prop This would enable even more flexibility or at least spare vendors from adding boilerplate code that, e.g., calculates TraceIds if there is no such concept in their own system. +In a (currently hypothetical) C++ implementation of OpenTelemetry (and possibly some other languages), +having the SpanContext be a "polymorphic", potentially derived class is a big difference to +the old SpanContext that might be implemented as a simple data structure with a few data fields. +These languages could opt to make SpanContext effectively a "smart pointer". +That is, the SpanContext could be implemented as a final class containing nothing but a reference to the actual SpanContextData (which is not final) and non-virtual functions that delegate to the corresponding virtual functions of the SpancontextData class. + +(Hypothetical) implementations of the OpenTelemetry specification that value performance highly over flexibility might choose to ignore this OTEP entirely, but these probably will not even be split into an API and SDK part. + ## Prior art and alternatives The topic was briefly discussed in From e47df04bead2853ce4eda06f95814e293a5bf7c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 4 Oct 2019 15:44:32 +0200 Subject: [PATCH 08/13] OpenTelemetry -> OpenTracing --- text/0000-unseal-spancontext.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index bfd5ffcc6..5111f3bd9 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -67,7 +67,7 @@ https://github.com/open-telemetry/opentelemetry-specification/pull/244 > `SpanContext` is represented as an interface, in order to be serializable into a wider variety of trace context wire formats. -In OpenTelemetry, the SpanContext was just an interface with very few public methods +In OpenTracing, the SpanContext was just an interface with very few public methods (in particular, it did not assume the existence of a thing such as Trace or Span ID): > The `SpanContext` is more of a "concept" than a useful piece of functionality at the generic OpenTracing layer. That said, it is of critical importance to OpenTracing *implementations* and does present a thin API of its own. Most OpenTracing users only interact with `SpanContext` via [**references**](https://github.com/opentracing/specification/blob/master/specification.md#references-between-spans) when starting new `Span`s, or when injecting/extracting a trace to/from some transport protocol. From 55b6cdeda751daa5bde927dc50accf9b9907b941 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Mon, 7 Oct 2019 09:47:24 +0200 Subject: [PATCH 09/13] More alternatives. --- text/0000-unseal-spancontext.md | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index 5111f3bd9..15e58074b 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -35,7 +35,9 @@ Additionally, all public APIs of the SpanContext must be overridable (they must not be public data fields in languages where these are not overridable, languages that require an explicit marker like `virtual` to make an API overridable must add it, etc.). -Existing propagators (which should be implemented in the SDK package or a separate package) can continue to directly construct a SpanContext. +Existing propagators +(which should be implemented in the SDK package or a separate package) +can continue to directly construct a SpanContext. API implementations that want to use a custom SpanContext implementation must either handle having a plain SpanContext as parent and having additional properties stripped upon extracting @@ -59,7 +61,7 @@ That is, the SpanContext could be implemented as a final class containing nothin (Hypothetical) implementations of the OpenTelemetry specification that value performance highly over flexibility might choose to ignore this OTEP entirely, but these probably will not even be split into an API and SDK part. -## Prior art and alternatives +## Prior art The topic was briefly discussed in https://github.com/open-telemetry/opentelemetry-specification/pull/244 @@ -75,7 +77,30 @@ In OpenTracing, the SpanContext was just an interface with very few public metho (quoted from https://github.com/opentracing/specification/blob/master/specification.md#spancontext) In principle, the same still applies to OpenTelemetry: -SpanContext has no functionality that is useful outside of propagation or implementations of Spans and Tracers. However, we chose to expose the "internal details" of the W3C context in the API. +SpanContext has no functionality that is useful outside of propagation or implementations of Spans and Tracers. +However, we chose to expose the "internal details" of the W3C context in the API. + +## Alternatives + +An alternative (or addition) to making SpanContext non-final is to reduce the places where it is used. +In particular, `inject` could take a full Span as parameter. +This would also make it harder to accidentally inject the parent SpanContext +(that was initially extracted). +If `inject(SpanContext)` is still needed, it could be provided as an additional overload +or renamed to `forward(SpanContext)`. +In the most radical implementation of this approach, SpanContext could entirely disappear, +if we also change `extract(source) -> SpanContext` + `startSpan(parent: SpanContext) -> Span` to +a single `startSpanWithRemoteParent(source, spanName, ...) -> Span`. + +Yet another alternative +(with different caveats) +would be to have SpanContext have a reference to the corresponding Span if any. +Then SDK-implementors could store any additional information in the Span. +Upon extraction they would either have to fall back to a `null` Span +and storing any data in the TraceState as strings +or they could set a special pseudo-Span to store information +(although that interferes with the reasonable assumption that +`SpanContext.getSpan() == null` iff `SpanContext.isRemote()`). ## Open questions From 4d09ced77a61377d8306af5bc04b896dd3177a06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Mon, 7 Oct 2019 11:24:56 +0200 Subject: [PATCH 10/13] Wording. --- text/0000-unseal-spancontext.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-unseal-spancontext.md b/text/0000-unseal-spancontext.md index 15e58074b..000394ab5 100644 --- a/text/0000-unseal-spancontext.md +++ b/text/0000-unseal-spancontext.md @@ -95,8 +95,8 @@ a single `startSpanWithRemoteParent(source, spanName, ...) -> Span`. Yet another alternative (with different caveats) would be to have SpanContext have a reference to the corresponding Span if any. -Then SDK-implementors could store any additional information in the Span. -Upon extraction they would either have to fall back to a `null` Span +Then API implementations could store any additional information in the Span. +Upon extraction they would either have to fall back to a `null` Span reference on the SpanContext and storing any data in the TraceState as strings or they could set a special pseudo-Span to store information (although that interferes with the reasonable assumption that From f20d87d07f401a28c4d0d4a5c546d47d76ff5be6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Mon, 7 Oct 2019 11:37:00 +0200 Subject: [PATCH 11/13] Assign PR No. to OTEP. --- text/{0000-unseal-spancontext.md => 0058-unseal-spancontext.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename text/{0000-unseal-spancontext.md => 0058-unseal-spancontext.md} (100%) diff --git a/text/0000-unseal-spancontext.md b/text/0058-unseal-spancontext.md similarity index 100% rename from text/0000-unseal-spancontext.md rename to text/0058-unseal-spancontext.md From d35a0628e935af90ed572c2c0b12f0a10b8680bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Tue, 15 Oct 2019 15:26:49 +0200 Subject: [PATCH 12/13] Purge C++ details, add alternative with SpanContext interface. --- text/0058-unseal-spancontext.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/text/0058-unseal-spancontext.md b/text/0058-unseal-spancontext.md index 000394ab5..1212499e3 100644 --- a/text/0058-unseal-spancontext.md +++ b/text/0058-unseal-spancontext.md @@ -15,7 +15,7 @@ On the other hand, a non-final SpanContext does have advantages for vendors' fle ## Explanation -SpanContext remains as a non-abstract class implemented in the API, but they are non-final. Note that the proposition +SpanContext remains as a non-abstract class implemented in the API, but it is non-final. Note that the proposition > SpanContexts are immutable. @@ -53,12 +53,6 @@ so that it becomes an object that is completely opaque (to everyone but the prop This would enable even more flexibility or at least spare vendors from adding boilerplate code that, e.g., calculates TraceIds if there is no such concept in their own system. -In a (currently hypothetical) C++ implementation of OpenTelemetry (and possibly some other languages), -having the SpanContext be a "polymorphic", potentially derived class is a big difference to -the old SpanContext that might be implemented as a simple data structure with a few data fields. -These languages could opt to make SpanContext effectively a "smart pointer". -That is, the SpanContext could be implemented as a final class containing nothing but a reference to the actual SpanContextData (which is not final) and non-virtual functions that delegate to the corresponding virtual functions of the SpancontextData class. - (Hypothetical) implementations of the OpenTelemetry specification that value performance highly over flexibility might choose to ignore this OTEP entirely, but these probably will not even be split into an API and SDK part. ## Prior art @@ -102,9 +96,15 @@ or they could set a special pseudo-Span to store information (although that interferes with the reasonable assumption that `SpanContext.getSpan() == null` iff `SpanContext.isRemote()`). -## Open questions +For new language libraries, the arguments of backward-compatiblity for a non-abstract non-final class does not exist. +Such languages may choose to implement SpanContext as an interface instead of a non-abstract class. +Among other things, +this may be used to improve performance +because a Span could implement that interface, thus saving allocations. +Some way for propagators to create a SpanContext that is not bound to a Span is still required to implement `extract` +(unless [OTEP #42 (Proposal to separate context propagation from observability)][OTEP-ctx] changes that). -None known yet. +[OTEP-ctx]: https://github.com/open-telemetry/oteps/pull/42 ## Future possibilities From 6da693cc07edf87801dbd0c88f9a53f47c6119ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Wed, 16 Oct 2019 10:22:35 +0200 Subject: [PATCH 13/13] Fix misspelling. --- text/0058-unseal-spancontext.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0058-unseal-spancontext.md b/text/0058-unseal-spancontext.md index 1212499e3..c0229b17b 100644 --- a/text/0058-unseal-spancontext.md +++ b/text/0058-unseal-spancontext.md @@ -96,7 +96,7 @@ or they could set a special pseudo-Span to store information (although that interferes with the reasonable assumption that `SpanContext.getSpan() == null` iff `SpanContext.isRemote()`). -For new language libraries, the arguments of backward-compatiblity for a non-abstract non-final class does not exist. +For new language libraries, the arguments of backward-compatibility for a non-abstract non-final class does not exist. Such languages may choose to implement SpanContext as an interface instead of a non-abstract class. Among other things, this may be used to improve performance