From 790135be50f73cc25917bfbf717bab6c3609db55 Mon Sep 17 00:00:00 2001 From: suraj kumar Date: Wed, 5 Jul 2023 20:00:58 +0530 Subject: [PATCH] Move span operation to Scope from Tracer (#8411) * Move span operation to Scope from Tracer Signed-off-by: suranjay * Add changelog entry Signed-off-by: suranjay * Change method to setError Signed-off-by: suranjay * Removed unused classes Signed-off-by: suranjay * Fix merge issue Signed-off-by: suranjay --------- Signed-off-by: suranjay Signed-off-by: Shivansh Arora --- CHANGELOG.md | 1 + .../telemetry/tracing/DefaultSpanScope.java | 70 ++++++++++++++++ .../telemetry/tracing/DefaultTracer.java | 38 +-------- .../opensearch/telemetry/tracing/Scope.java | 26 ------ .../telemetry/tracing/ScopeImpl.java | 33 -------- .../opensearch/telemetry/tracing/Span.java | 7 ++ .../telemetry/tracing/SpanScope.java | 74 +++++++++++++++++ .../opensearch/telemetry/tracing/Tracer.java | 42 +--------- .../telemetry/tracing/noop/NoopSpanScope.java | 57 +++++++++++++ .../telemetry/tracing/noop/NoopTracer.java | 47 +---------- .../tracing/DefaultSpanScopeTests.java | 79 +++++++++++++++++++ .../telemetry/tracing/DefaultTracerTests.java | 47 +---------- .../telemetry/tracing/OTelSpan.java | 6 ++ .../telemetry/tracing/TracerFactoryTests.java | 2 +- .../test/telemetry/tracing/MockSpan.java | 4 + 15 files changed, 309 insertions(+), 224 deletions(-) create mode 100644 libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java delete mode 100644 libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Scope.java delete mode 100644 libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/ScopeImpl.java create mode 100644 libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanScope.java create mode 100644 libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java create mode 100644 libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultSpanScopeTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a95531887c65..bb571cc21cd5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) - Pass localNode info to all plugins on node start ([#7919](https://github.com/opensearch-project/OpenSearch/pull/7919)) - Improved performance of parsing floating point numbers ([#7909](https://github.com/opensearch-project/OpenSearch/pull/7909)) +- Move span actions to Scope ([#8411](https://github.com/opensearch-project/OpenSearch/pull/8411)) ### Deprecated diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java new file mode 100644 index 0000000000000..58e9e0abad739 --- /dev/null +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java @@ -0,0 +1,70 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.telemetry.tracing; + +import java.util.function.Consumer; + +/** + * Default implementation of Scope + */ +public class DefaultSpanScope implements SpanScope { + + private final Span span; + + private final Consumer onCloseConsumer; + + /** + * Creates Scope instance for the given span + * + * @param span underlying span + * @param onCloseConsumer consumer to execute on scope close + */ + public DefaultSpanScope(Span span, Consumer onCloseConsumer) { + this.span = span; + this.onCloseConsumer = onCloseConsumer; + } + + @Override + public void addSpanAttribute(String key, String value) { + span.addAttribute(key, value); + } + + @Override + public void addSpanAttribute(String key, long value) { + span.addAttribute(key, value); + } + + @Override + public void addSpanAttribute(String key, double value) { + span.addAttribute(key, value); + } + + @Override + public void addSpanAttribute(String key, boolean value) { + span.addAttribute(key, value); + } + + @Override + public void addSpanEvent(String event) { + span.addEvent(event); + } + + @Override + public void setError(Exception exception) { + span.setError(exception); + } + + /** + * Executes the runnable to end the scope + */ + @Override + public void close() { + onCloseConsumer.accept(span); + } +} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java index ab9110af7c3ab..783edd238c1c2 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java @@ -13,8 +13,8 @@ /** * - * The default tracer implementation. This class implements the basic logic for span lifecycle and its state management. - * It also handles tracing context propagation between spans. + * The default tracer implementation. It handles tracing context propagation between spans by maintaining + * current active span in its storage * * */ @@ -36,41 +36,11 @@ public DefaultTracer(TracingTelemetry tracingTelemetry, TracerContextStorage endSpan(span)); - } - - @Override - public void addSpanAttribute(String key, String value) { - Span currentSpan = getCurrentSpan(); - currentSpan.addAttribute(key, value); - } - - @Override - public void addSpanAttribute(String key, long value) { - Span currentSpan = getCurrentSpan(); - currentSpan.addAttribute(key, value); - } - - @Override - public void addSpanAttribute(String key, double value) { - Span currentSpan = getCurrentSpan(); - currentSpan.addAttribute(key, value); - } - - @Override - public void addSpanAttribute(String key, boolean value) { - Span currentSpan = getCurrentSpan(); - currentSpan.addAttribute(key, value); - } - - @Override - public void addSpanEvent(String event) { - Span currentSpan = getCurrentSpan(); - currentSpan.addEvent(event); + return new DefaultSpanScope(span, (scopeSpan) -> endSpan(scopeSpan)); } @Override diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Scope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Scope.java deleted file mode 100644 index 52f4eaf648eea..0000000000000 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Scope.java +++ /dev/null @@ -1,26 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.telemetry.tracing; - -/** - * An auto-closeable that represents scope of the span. - * It is recommended that you use this class with a try-with-resources block: - */ -public interface Scope extends AutoCloseable { - /** - * No-op Scope implementation - */ - Scope NO_OP = () -> {}; - - /** - * closes the scope - */ - @Override - void close(); -} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/ScopeImpl.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/ScopeImpl.java deleted file mode 100644 index 30a7ac7fa90e7..0000000000000 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/ScopeImpl.java +++ /dev/null @@ -1,33 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.telemetry.tracing; - -/** - * Executes the runnable on close - */ -public class ScopeImpl implements Scope { - - private Runnable runnableOnClose; - - /** - * Creates Scope instance - * @param runnableOnClose runnable to execute on scope close - */ - public ScopeImpl(Runnable runnableOnClose) { - this.runnableOnClose = runnableOnClose; - } - - /** - * Executes the runnable to end the scope - */ - @Override - public void close() { - runnableOnClose.run(); - } -} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java index 0710b8a22a37f..d60b4e60adece 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java @@ -62,6 +62,13 @@ public interface Span { */ void addAttribute(String key, Boolean value); + /** + * Records error in the span + * + * @param exception exception to be recorded + */ + void setError(Exception exception); + /** * Adds an event in the span * diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanScope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanScope.java new file mode 100644 index 0000000000000..cf67165d889bc --- /dev/null +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanScope.java @@ -0,0 +1,74 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.telemetry.tracing; + +import org.opensearch.telemetry.tracing.noop.NoopSpanScope; + +/** + * An auto-closeable that represents scope of the span. + * It provides interface for all the span operations. + */ +public interface SpanScope extends AutoCloseable { + /** + * No-op Scope implementation + */ + SpanScope NO_OP = new NoopSpanScope(); + + /** + * Adds string attribute to the {@link Span}. + * + * @param key attribute key + * @param value attribute value + */ + void addSpanAttribute(String key, String value); + + /** + * Adds long attribute to the {@link Span}. + * + * @param key attribute key + * @param value attribute value + */ + void addSpanAttribute(String key, long value); + + /** + * Adds double attribute to the {@link Span}. + * + * @param key attribute key + * @param value attribute value + */ + void addSpanAttribute(String key, double value); + + /** + * Adds boolean attribute to the {@link Span}. + * + * @param key attribute key + * @param value attribute value + */ + void addSpanAttribute(String key, boolean value); + + /** + * Adds an event to the {@link Span}. + * + * @param event event name + */ + void addSpanEvent(String event); + + /** + * Records error in the span + * + * @param exception exception to be recorded + */ + void setError(Exception exception); + + /** + * closes the scope + */ + @Override + void close(); +} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java index fcc091eb39c48..d422b58aa0a9f 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java @@ -11,7 +11,7 @@ import java.io.Closeable; /** - * Tracer is the interface used to create a {@link Span} and interact with current active {@link Span}. + * Tracer is the interface used to create a {@link Span} * It automatically handles the context propagation between threads, tasks, nodes etc. * * All methods on the Tracer object are multi-thread safe. @@ -24,44 +24,6 @@ public interface Tracer extends Closeable { * @param spanName span name * @return scope of the span, must be closed with explicit close or with try-with-resource */ - Scope startSpan(String spanName); + SpanScope startSpan(String spanName); - /** - * Adds string attribute to the current active {@link Span}. - * - * @param key attribute key - * @param value attribute value - */ - void addSpanAttribute(String key, String value); - - /** - * Adds long attribute to the current active {@link Span}. - * - * @param key attribute key - * @param value attribute value - */ - void addSpanAttribute(String key, long value); - - /** - * Adds double attribute to the current active {@link Span}. - * - * @param key attribute key - * @param value attribute value - */ - void addSpanAttribute(String key, double value); - - /** - * Adds boolean attribute to the current active {@link Span}. - * - * @param key attribute key - * @param value attribute value - */ - void addSpanAttribute(String key, boolean value); - - /** - * Adds an event to the current active {@link Span}. - * - * @param event event name - */ - void addSpanEvent(String event); } diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java new file mode 100644 index 0000000000000..c0dbaf65ba48b --- /dev/null +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java @@ -0,0 +1,57 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.telemetry.tracing.noop; + +import org.opensearch.telemetry.tracing.SpanScope; + +/** + * No-op implementation of SpanScope + */ +public final class NoopSpanScope implements SpanScope { + + /** + * No-args constructor + */ + public NoopSpanScope() {} + + @Override + public void addSpanAttribute(String key, String value) { + + } + + @Override + public void addSpanAttribute(String key, long value) { + + } + + @Override + public void addSpanAttribute(String key, double value) { + + } + + @Override + public void addSpanAttribute(String key, boolean value) { + + } + + @Override + public void addSpanEvent(String event) { + + } + + @Override + public void setError(Exception exception) { + + } + + @Override + public void close() { + + } +} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java index 18fc60e41e54d..a66cbcf4fef52 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java @@ -8,7 +8,7 @@ package org.opensearch.telemetry.tracing.noop; -import org.opensearch.telemetry.tracing.Scope; +import org.opensearch.telemetry.tracing.SpanScope; import org.opensearch.telemetry.tracing.Tracer; /** @@ -24,49 +24,8 @@ public class NoopTracer implements Tracer { private NoopTracer() {} @Override - public Scope startSpan(String spanName) { - return Scope.NO_OP; - } - - /** - * @param key attribute key - * @param value attribute value - */ - @Override - public void addSpanAttribute(String key, String value) { - - } - - /** - * @param key attribute key - * @param value attribute value - */ - @Override - public void addSpanAttribute(String key, long value) { - - } - - /** - * @param key attribute key - * @param value attribute value - */ - @Override - public void addSpanAttribute(String key, double value) { - - } - - /** - * @param key attribute key - * @param value attribute value - */ - @Override - public void addSpanAttribute(String key, boolean value) { - - } - - @Override - public void addSpanEvent(String event) { - + public SpanScope startSpan(String spanName) { + return SpanScope.NO_OP; } @Override diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultSpanScopeTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultSpanScopeTests.java new file mode 100644 index 0000000000000..eea6b77ce6e1e --- /dev/null +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultSpanScopeTests.java @@ -0,0 +1,79 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.telemetry.tracing; + +import org.opensearch.test.OpenSearchTestCase; + +import java.util.function.Consumer; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +public class DefaultSpanScopeTests extends OpenSearchTestCase { + + @SuppressWarnings("unchecked") + public void testClose() { + Span mockSpan = mock(Span.class); + Consumer mockConsumer = mock(Consumer.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, mockConsumer); + defaultSpanScope.close(); + + verify(mockConsumer).accept(mockSpan); + } + + public void testAddSpanAttributeString() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanAttribute("key", "value"); + + verify(mockSpan).addAttribute("key", "value"); + } + + public void testAddSpanAttributeLong() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanAttribute("key", 1L); + + verify(mockSpan).addAttribute("key", 1L); + } + + public void testAddSpanAttributeDouble() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanAttribute("key", 1.0); + + verify(mockSpan).addAttribute("key", 1.0); + } + + public void testAddSpanAttributeBoolean() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanAttribute("key", true); + + verify(mockSpan).addAttribute("key", true); + } + + public void testAddEvent() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanEvent("eventName"); + + verify(mockSpan).addEvent("eventName"); + } + + public void testSetError() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + Exception ex = new Exception("error"); + defaultSpanScope.setError(ex); + + verify(mockSpan).setError(ex); + } + +} diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java index f0e8f3c2e2344..2b7a379b0051a 100644 --- a/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java @@ -45,57 +45,12 @@ public void testCreateSpan() { public void testEndSpanByClosingScope() { DefaultTracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - try (Scope scope = defaultTracer.startSpan("span_name")) { + try (SpanScope spanScope = defaultTracer.startSpan("span_name")) { verify(mockTracerContextStorage).put(TracerContextStorage.CURRENT_SPAN, mockSpan); } verify(mockTracerContextStorage).put(TracerContextStorage.CURRENT_SPAN, mockParentSpan); } - public void testAddSpanAttributeString() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanAttribute("key", "value"); - - verify(mockSpan).addAttribute("key", "value"); - } - - public void testAddSpanAttributeLong() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanAttribute("key", 1L); - - verify(mockSpan).addAttribute("key", 1L); - } - - public void testAddSpanAttributeDouble() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanAttribute("key", 1.0); - - verify(mockSpan).addAttribute("key", 1.0); - } - - public void testAddSpanAttributeBoolean() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanAttribute("key", true); - - verify(mockSpan).addAttribute("key", true); - } - - public void testAddEvent() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanEvent("eventName"); - - verify(mockSpan).addEvent("eventName"); - } - public void testClose() throws IOException { Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); diff --git a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java index 23a2d9baa3e6e..ba63df4ae47a1 100644 --- a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java +++ b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java @@ -9,6 +9,7 @@ package org.opensearch.telemetry.tracing; import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.StatusCode; /** * Default implementation of {@link Span} using Otel span. It keeps a reference of OpenTelemetry Span and handles span @@ -48,6 +49,11 @@ public void addAttribute(String key, Boolean value) { delegateSpan.setAttribute(key, value); } + @Override + public void setError(Exception exception) { + delegateSpan.setStatus(StatusCode.ERROR, exception.getMessage()); + } + @Override public void addEvent(String event) { delegateSpan.addEvent(event); diff --git a/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java b/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java index 7968c6c43afb4..df9cdd6669d23 100644 --- a/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java +++ b/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java @@ -45,7 +45,7 @@ public void testGetTracerWithTracingDisabledReturnsNoopTracer() { Tracer tracer = tracerFactory.getTracer(); assertTrue(tracer instanceof NoopTracer); - assertTrue(tracer.startSpan("foo") == Scope.NO_OP); + assertTrue(tracer.startSpan("foo") == SpanScope.NO_OP); } public void testGetTracerWithTracingEnabledReturnsDefaultTracer() { diff --git a/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockSpan.java b/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockSpan.java index 4779d9796e11e..876145f6bf653 100644 --- a/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockSpan.java +++ b/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockSpan.java @@ -142,6 +142,10 @@ public Long getEndTime() { return endTime; } + public void setError(Exception exception) { + putMetadata("ERROR", exception.getMessage()); + } + private static class IdGenerator { private static String generateSpanId() { long id = randomSupplier.get().nextLong();