-
Notifications
You must be signed in to change notification settings - Fork 852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow for simpler creation of start-only and end-only SpanProcessors. #5923
Merged
jack-berg
merged 6 commits into
open-telemetry:main
from
breedx-splk:span_processor_helpers
Dec 4, 2023
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
b287049
allow for simpler creation of start-only and end-only SpanProcessors.
breedx-splk 5bddf1c
Create helper classes instead of touching interface.
breedx-splk 27c93bb
fix tests
breedx-splk 32c12d3
custom functional interfaces
breedx-splk e5062ff
move to incubator
breedx-splk 6b97a5f
spotless
breedx-splk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
49 changes: 49 additions & 0 deletions
49
...ator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/OnEndSpanProcessor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.sdk.extension.incubator.trace; | ||
|
||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.sdk.trace.ReadWriteSpan; | ||
import io.opentelemetry.sdk.trace.ReadableSpan; | ||
import io.opentelemetry.sdk.trace.SpanProcessor; | ||
|
||
/** A SpanProcessor implementation that is only capable of processing spans when they end. */ | ||
public final class OnEndSpanProcessor implements SpanProcessor { | ||
private final OnEnd onEnd; | ||
|
||
private OnEndSpanProcessor(OnEnd onEnd) { | ||
this.onEnd = onEnd; | ||
} | ||
|
||
static SpanProcessor create(OnEnd onEnd) { | ||
return new OnEndSpanProcessor(onEnd); | ||
} | ||
|
||
@Override | ||
public void onEnd(ReadableSpan span) { | ||
onEnd.apply(span); | ||
} | ||
|
||
@Override | ||
public boolean isEndRequired() { | ||
return true; | ||
} | ||
|
||
@Override | ||
public void onStart(Context parentContext, ReadWriteSpan span) { | ||
// nop | ||
} | ||
|
||
Check warning on line 39 in sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/OnEndSpanProcessor.java Codecov / codecov/patchsdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/OnEndSpanProcessor.java#L38-L39
|
||
@Override | ||
public boolean isStartRequired() { | ||
return false; | ||
} | ||
|
||
@FunctionalInterface | ||
public interface OnEnd { | ||
void apply(ReadableSpan span); | ||
} | ||
} |
50 changes: 50 additions & 0 deletions
50
...or/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/OnStartSpanProcessor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.sdk.extension.incubator.trace; | ||
|
||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.sdk.trace.ReadWriteSpan; | ||
import io.opentelemetry.sdk.trace.ReadableSpan; | ||
import io.opentelemetry.sdk.trace.SpanProcessor; | ||
|
||
/** A SpanProcessor that only handles onStart(). */ | ||
public final class OnStartSpanProcessor implements SpanProcessor { | ||
|
||
private final OnStart onStart; | ||
|
||
private OnStartSpanProcessor(OnStart onStart) { | ||
this.onStart = onStart; | ||
} | ||
|
||
public static SpanProcessor create(OnStart onStart) { | ||
return new OnStartSpanProcessor(onStart); | ||
} | ||
|
||
@Override | ||
public void onStart(Context parentContext, ReadWriteSpan span) { | ||
onStart.apply(parentContext, span); | ||
} | ||
|
||
@Override | ||
public boolean isStartRequired() { | ||
return true; | ||
} | ||
|
||
@Override | ||
public void onEnd(ReadableSpan span) { | ||
// nop | ||
} | ||
|
||
@Override | ||
public boolean isEndRequired() { | ||
return false; | ||
} | ||
|
||
@FunctionalInterface | ||
public interface OnStart { | ||
void apply(Context context, ReadWriteSpan span); | ||
} | ||
} |
31 changes: 31 additions & 0 deletions
31
.../src/test/java/io/opentelemetry/sdk/extension/incubator/trace/OnEndSpanProcessorTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.sdk.extension.incubator.trace; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
|
||
import io.opentelemetry.sdk.trace.ReadWriteSpan; | ||
import io.opentelemetry.sdk.trace.ReadableSpan; | ||
import io.opentelemetry.sdk.trace.SpanProcessor; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class OnEndSpanProcessorTest { | ||
|
||
@Test | ||
void endOnly() { | ||
AtomicReference<ReadableSpan> seenSpan = new AtomicReference<>(); | ||
ReadWriteSpan inputSpan = mock(ReadWriteSpan.class); | ||
|
||
SpanProcessor processor = OnEndSpanProcessor.create(seenSpan::set); | ||
|
||
assertThat(processor.isStartRequired()).isFalse(); | ||
assertThat(processor.isEndRequired()).isTrue(); | ||
processor.onEnd(inputSpan); | ||
assertThat(seenSpan.get()).isSameAs(inputSpan); | ||
} | ||
} |
39 changes: 39 additions & 0 deletions
39
...rc/test/java/io/opentelemetry/sdk/extension/incubator/trace/OnStartSpanProcessorTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.sdk.extension.incubator.trace; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
|
||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.sdk.trace.ReadWriteSpan; | ||
import io.opentelemetry.sdk.trace.SpanProcessor; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class OnStartSpanProcessorTest { | ||
|
||
@Test | ||
void startOnly() { | ||
AtomicReference<Context> seenContext = new AtomicReference<>(); | ||
AtomicReference<ReadWriteSpan> seenSpan = new AtomicReference<>(); | ||
Context context = mock(Context.class); | ||
ReadWriteSpan inputSpan = mock(ReadWriteSpan.class); | ||
|
||
SpanProcessor processor = | ||
OnStartSpanProcessor.create( | ||
(ctx, span) -> { | ||
seenContext.set(ctx); | ||
seenSpan.set(span); | ||
}); | ||
|
||
assertThat(processor.isStartRequired()).isTrue(); | ||
assertThat(processor.isEndRequired()).isFalse(); | ||
processor.onStart(context, inputSpan); | ||
assertThat(seenContext.get()).isSameAs(context); | ||
assertThat(seenSpan.get()).isSameAs(inputSpan); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply or onEnd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used
apply
for both OnStart and OnEnd. I read it as a functional interface, so an implementation or lambda instance of OnEnd is the function and the function gets applied. At least that's how my brain thinks of it.I think the name isn't hugely important here, but
OnEnd.onEnd()
seems a little silly. I think one could also make the case that it's a consumer, so it should be calledaccept()
, but thenOnStart
has two args.I guess I think apply is the most straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this too silly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's what I suggested in this comment @trask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for the link 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not, but it does allow people to still do silly things with it. :)