Skip to content
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.

Sign up for GitHub

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

Set custom gRPC client/server span name extractor #5244

Merged
merged 8 commits into from
Feb 24, 2022
Merged

Set custom gRPC client/server span name extractor #5244

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
461c870
Set custom gRPC client/server span name extractor
eugeniyk Jan 26, 2022
4f22154
Fix imports
eugeniyk Jan 26, 2022
f748538
Fix compilation
eugeniyk Jan 27, 2022
d092af4
Fix style rule
eugeniyk Feb 4, 2022
05aac61
Update to use Functions instead
eugeniyk Feb 17, 2022
91ab31d
Fix formatting
eugeniyk Feb 17, 2022
822f805
Merge remote-tracking branch 'origin/main' into add-grpc-name-extract…
eugeniyk Feb 17, 2022
1319c74
Merge remote-tracking branch 'origin/main' into add-grpc-name-extract…
eugeniyk Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions .../library/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/GrpcTracingBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder;
import io.opentelemetry.instrumentation.api.instrumenter.PeerServiceAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesExtractor;
import io.opentelemetry.instrumentation.grpc.v1_6.internal.GrpcNetClientAttributesGetter;
Expand All @@ -29,6 +30,8 @@ public final class GrpcTracingBuilder {

private final OpenTelemetry openTelemetry;
@Nullable private String peerService;
@Nullable private SpanNameExtractor<? super GrpcRequest> clientSpanNameExtractor;
@Nullable private SpanNameExtractor<? super GrpcRequest> serverSpanNameExtractor;

private final List<AttributesExtractor<? super GrpcRequest, ? super Status>>
additionalExtractors = new ArrayList<>();
Expand All @@ -49,9 +52,28 @@ public GrpcTracingBuilder addAttributeExtractor(
return this;
}

/**
* Sets custom client {@link SpanNameExtractor}
*/
public GrpcTracingBuilder setClientSpanNameExtractor(
SpanNameExtractor<? super GrpcRequest> clientSpanNameExtractor) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of just passing the SpanNameExtractor, WDYT about passing a Function<SpanNameExtractor<GrpcRequest>, ? extends SpanNameExtractor<? super GrpcRequest>>? (similar to how the Armeria instrumentation allows modifying the SpanStatusExtractor)
With a transformer function you can still make use of the original span name extractors - or reject them altogether and return your own, if you wish to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds interesting
Chaining status code transformations makes more sense imo than extracting span name (which is a string, doubt you'd like to have pattern matching on it)

Or what's the use case you have in mind? One could be if length is more that X use shorter version of method name; since default impl is pretty straight forward I'd stick to the current version, + it gives api consistency with other methods

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or what's the use case you have in mind? One could be if length is more that X use shorter version of method name;

That's one option. I was thinking of a case where you might decide to fall back on the default naming scheme based on the request, e.g. (request) -> shouldRename(request) ? customName(request) : originalSpanNameExtractor.extract(request)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trask what do you think? current way as api consistency vs suggested flexibility?

I'm ok with both

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's go with @mateuszrzeszutek's proposal, thx!

this.clientSpanNameExtractor = clientSpanNameExtractor;
return this;
}

/**
* Sets custom server {@link SpanNameExtractor}
*/
public GrpcTracingBuilder setServerSpanNameExtractor(
SpanNameExtractor<? super GrpcRequest> serverSpanNameExtractor) {
this.serverSpanNameExtractor = serverSpanNameExtractor;
return this;
}

/** Sets the {@code peer.service} attribute for http client spans. */
public void setPeerService(String peerService) {
public GrpcTracingBuilder setPeerService(String peerService) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing existing builder method

this.peerService = peerService;
return this;
}

/**
Expand All @@ -67,10 +89,20 @@ public GrpcTracingBuilder setCaptureExperimentalSpanAttributes(

/** Returns a new {@link GrpcTracing} with the settings of this {@link GrpcTracingBuilder}. */
public GrpcTracing build() {
SpanNameExtractor<? super GrpcRequest> clientSpanNameExtractor = this.clientSpanNameExtractor;
if (clientSpanNameExtractor == null) {
clientSpanNameExtractor = new GrpcSpanNameExtractor();
}

SpanNameExtractor<? super GrpcRequest> serverSpanNameExtractor = this.serverSpanNameExtractor;
if (serverSpanNameExtractor == null) {
serverSpanNameExtractor = new GrpcSpanNameExtractor();
}

InstrumenterBuilder<GrpcRequest, Status> clientInstrumenterBuilder =
Instrumenter.builder(openTelemetry, INSTRUMENTATION_NAME, new GrpcSpanNameExtractor());
Instrumenter.builder(openTelemetry, INSTRUMENTATION_NAME, clientSpanNameExtractor);
InstrumenterBuilder<GrpcRequest, Status> serverInstrumenterBuilder =
Instrumenter.builder(openTelemetry, INSTRUMENTATION_NAME, new GrpcSpanNameExtractor());
Instrumenter.builder(openTelemetry, INSTRUMENTATION_NAME, serverSpanNameExtractor);

Stream.of(clientInstrumenterBuilder, serverInstrumenterBuilder)
.forEach(
Expand Down