From 6dbe06839d170426434d8ba1921db01b8cac654b Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Fri, 10 May 2024 17:19:48 -0300 Subject: [PATCH 1/8] record links without span context --- .../src/opentelemetry/sdk/trace/__init__.py | 7 ++++- opentelemetry-sdk/tests/trace/test_trace.py | 29 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 55a375315b..87e57aa834 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -867,7 +867,12 @@ def add_link( context: SpanContext, attributes: types.Attributes = None, ) -> None: - if context is None or not context.is_valid: + + if ( + context is None + or not context.is_valid + and not (attributes or context.trace_state) + ): return attributes = BoundedAttributes( diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 72c7095da7..df2e1c3231 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -944,6 +944,35 @@ def test_add_link_with_invalid_span_context(self): self.assertEqual(len(root.links), 0) + def test_add_link_with_invalid_span_context_record(self): + invalid_context = trace_api.INVALID_SPAN_CONTEXT + + with self.tracer.start_as_current_span("root") as root: + root.add_link(invalid_context, {"name": "neighbor"}) + self.assertEqual(len(root.links), 1) + self.assertEqual(root.links[0].attributes, {"name": "neighbor"}) + + invalid_context = trace.SpanContext( + trace_api.INVALID_TRACE_ID, + trace_api.INVALID_SPAN_ID, + is_remote=False, + trace_state="a=b", + ) + + with self.tracer.start_as_current_span("root") as root: + root.add_link(invalid_context) + self.assertEqual(len(root.links), 1) + self.assertEqual(root.links[0].context.trace_state, "a=b") + + with self.tracer.start_as_current_span("root") as root: + root.add_link(invalid_context) + root.add_link(invalid_context, {"name": "neighbor"}) + self.assertEqual(len(root.links), 2) + + with self.tracer.start_as_current_span("root") as root: + root.add_link(None) + self.assertEqual(len(root.links), 0) + def test_update_name(self): with self.tracer.start_as_current_span("root") as root: # name From 94d697de8cd37c446534cefc68f2d452fe64cf21 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Sat, 11 May 2024 11:39:18 -0300 Subject: [PATCH 2/8] docs: update add_link docs --- opentelemetry-api/src/opentelemetry/trace/span.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/opentelemetry-api/src/opentelemetry/trace/span.py b/opentelemetry-api/src/opentelemetry/trace/span.py index 5d46ffcb4a..4afc4d520a 100644 --- a/opentelemetry-api/src/opentelemetry/trace/span.py +++ b/opentelemetry-api/src/opentelemetry/trace/span.py @@ -127,7 +127,8 @@ def add_link( # pylint: disable=no-self-use Adds a single `Link` with the `SpanContext` of the span to link to and, optionally, attributes passed as arguments. Implementations may ignore - calls with an invalid span context. + calls with an invalid span context if both attributes and TraceState + are empty. Note: It is preferred to add links at span creation, instead of calling this method later since samplers can only consider information already From 533f5f328ea8b0e38c589d00760aa4f3dbc6472e Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Sat, 11 May 2024 11:40:05 -0300 Subject: [PATCH 3/8] feat: create _is_valid_link function --- .../src/opentelemetry/sdk/trace/__init__.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 87e57aa834..a98d652ab8 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -347,6 +347,14 @@ def wrapper(self, *args, **kwargs): return wrapper +def _is_valid_link(context: SpanContext, attributes: types.Attributes) -> bool: + return ( + context.is_valid or (attributes or context.trace_state) + if context + else False + ) + + class ReadableSpan: """Provides read-only access to span attributes. @@ -868,11 +876,7 @@ def add_link( attributes: types.Attributes = None, ) -> None: - if ( - context is None - or not context.is_valid - and not (attributes or context.trace_state) - ): + if not _is_valid_link(context, attributes): return attributes = BoundedAttributes( From ef288e8caee4baf3f614a4e2dc40ec97063878fd Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Sat, 11 May 2024 12:01:11 -0300 Subject: [PATCH 4/8] add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b25747004a..2d813c34ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#3823] (https://github.com/open-telemetry/opentelemetry-python/pull/3823)) - Add span flags to OTLP spans and links ([#3881](https://github.com/open-telemetry/opentelemetry-python/pull/3881)) +- Record links with invalid SpanContext + ([#3917](https://github.com/open-telemetry/opentelemetry-python/pull/3917/)) ## Version 1.24.0/0.45b0 (2024-03-28) From 3088259e938c0613f697bba665dfa7eb4fdf37eb Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Sat, 11 May 2024 20:54:00 -0300 Subject: [PATCH 5/8] rename tests --- opentelemetry-sdk/tests/trace/test_trace.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index df2e1c3231..88a2b9c986 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -941,10 +941,10 @@ def test_add_link_with_invalid_span_context(self): with self.tracer.start_as_current_span("root") as root: root.add_link(other_context) - + root.add_link(None) self.assertEqual(len(root.links), 0) - def test_add_link_with_invalid_span_context_record(self): + def test_add_link_with_invalid_span_context_with_attributes(self): invalid_context = trace_api.INVALID_SPAN_CONTEXT with self.tracer.start_as_current_span("root") as root: @@ -952,6 +952,7 @@ def test_add_link_with_invalid_span_context_record(self): self.assertEqual(len(root.links), 1) self.assertEqual(root.links[0].attributes, {"name": "neighbor"}) + def test_add_link_with_invalid_span_context_with_tracestate(self): invalid_context = trace.SpanContext( trace_api.INVALID_TRACE_ID, trace_api.INVALID_SPAN_ID, @@ -964,15 +965,6 @@ def test_add_link_with_invalid_span_context_record(self): self.assertEqual(len(root.links), 1) self.assertEqual(root.links[0].context.trace_state, "a=b") - with self.tracer.start_as_current_span("root") as root: - root.add_link(invalid_context) - root.add_link(invalid_context, {"name": "neighbor"}) - self.assertEqual(len(root.links), 2) - - with self.tracer.start_as_current_span("root") as root: - root.add_link(None) - self.assertEqual(len(root.links), 0) - def test_update_name(self): with self.tracer.start_as_current_span("root") as root: # name From 3fbd010b2db80882fbeeb0f782b618400ae6bdb9 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Sat, 11 May 2024 20:59:06 -0300 Subject: [PATCH 6/8] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d813c34ae..ff826d1570 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#3823] (https://github.com/open-telemetry/opentelemetry-python/pull/3823)) - Add span flags to OTLP spans and links ([#3881](https://github.com/open-telemetry/opentelemetry-python/pull/3881)) -- Record links with invalid SpanContext +- Record links with invalid SpanContext if either attributes or TraceState are not empty ([#3917](https://github.com/open-telemetry/opentelemetry-python/pull/3917/)) ## Version 1.24.0/0.45b0 (2024-03-28) From b4fb5cbc73577b5b0877da490271aac7976a7795 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Mon, 13 May 2024 08:55:14 -0300 Subject: [PATCH 7/8] improve link validation to make sure return a bool --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index a98d652ab8..99c0825f9e 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -348,10 +348,8 @@ def wrapper(self, *args, **kwargs): def _is_valid_link(context: SpanContext, attributes: types.Attributes) -> bool: - return ( - context.is_valid or (attributes or context.trace_state) - if context - else False + return bool( + context and (context.is_valid or (attributes or context.trace_state)) ) From 293019cad38e14e96cdf4609bed74de5d7c600b8 Mon Sep 17 00:00:00 2001 From: emdneto <9735060+emdneto@users.noreply.github.com> Date: Tue, 21 May 2024 16:14:49 -0300 Subject: [PATCH 8/8] test trace_state rename var --- opentelemetry-sdk/tests/trace/test_trace.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 88a2b9c986..bedc071caf 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -957,13 +957,13 @@ def test_add_link_with_invalid_span_context_with_tracestate(self): trace_api.INVALID_TRACE_ID, trace_api.INVALID_SPAN_ID, is_remote=False, - trace_state="a=b", + trace_state="foo=bar", ) with self.tracer.start_as_current_span("root") as root: root.add_link(invalid_context) self.assertEqual(len(root.links), 1) - self.assertEqual(root.links[0].context.trace_state, "a=b") + self.assertEqual(root.links[0].context.trace_state, "foo=bar") def test_update_name(self): with self.tracer.start_as_current_span("root") as root: