-
Notifications
You must be signed in to change notification settings - Fork 892
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
Rename ServerSpanNaming to HttpRouteHolder #5211
Merged
trask
merged 2 commits into
open-telemetry:main
from
mateuszrzeszutek:rename-ServerSpanNaming
Jan 25, 2022
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
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
21 changes: 21 additions & 0 deletions
21
...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteGetter.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,21 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter.http; | ||
|
||
import io.opentelemetry.context.Context; | ||
import javax.annotation.Nullable; | ||
|
||
/** An interface for getting the {@code http.route} attribute. */ | ||
@FunctionalInterface | ||
public interface HttpRouteGetter<T> { | ||
|
||
/** | ||
* Returns the {@code http.route} attribute extracted from {@code context} and {@code arg}; or | ||
* {@code null} if it was not found. | ||
*/ | ||
@Nullable | ||
String get(Context context, T arg); | ||
} |
21 changes: 21 additions & 0 deletions
21
...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteGetter2.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,21 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter.http; | ||
|
||
import io.opentelemetry.context.Context; | ||
import javax.annotation.Nullable; | ||
|
||
/** An interface for getting the {@code http.route} attribute. */ | ||
@FunctionalInterface | ||
public interface HttpRouteGetter2<T, U> { | ||
|
||
/** | ||
* Returns the {@code http.route} attribute extracted from {@code context}, {@code arg1} and | ||
* {@code arg2}; or {@code null} if it was not found. | ||
*/ | ||
@Nullable | ||
String get(Context context, T arg1, U arg2); | ||
} |
157 changes: 157 additions & 0 deletions
157
...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.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,157 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter.http; | ||
|
||
import io.opentelemetry.api.trace.Span; | ||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.context.ContextKey; | ||
import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer; | ||
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; | ||
import io.opentelemetry.instrumentation.api.server.ServerSpan; | ||
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; | ||
import javax.annotation.Nullable; | ||
|
||
/** | ||
* A helper class that keeps track of the {@code http.route} attribute value during HTTP server | ||
* request processing. | ||
* | ||
* <p>Usually the route is not accessible when the request processing starts; and needs to be set | ||
* later, after the instrumented operation starts. This class provides several static methods that | ||
* allow the isntrumentation author to provide the matching HTTP route to the instrumentation when | ||
* it is discovered. | ||
*/ | ||
public final class HttpRouteHolder { | ||
|
||
private static final ContextKey<HttpRouteHolder> CONTEXT_KEY = | ||
ContextKey.named("opentelemetry-http-server-route-key"); | ||
|
||
/** | ||
* Returns a {@link ContextCustomizer} that initializes a {@link HttpRouteHolder} in the {@link | ||
* Context} returned from {@link Instrumenter#start(Context, Object)}. | ||
*/ | ||
public static <REQUEST> ContextCustomizer<REQUEST> get() { | ||
return (context, request, startAttributes) -> { | ||
if (context.get(CONTEXT_KEY) != null) { | ||
return context; | ||
} | ||
return context.with(CONTEXT_KEY, new HttpRouteHolder()); | ||
}; | ||
} | ||
|
||
private volatile int updatedBySourceOrder = 0; | ||
@Nullable private volatile String route; | ||
|
||
private HttpRouteHolder() {} | ||
|
||
/** | ||
* Updates the {@code http.route} attribute in the received {@code context}. | ||
* | ||
* <p>If there is a server span in the context, and the context has been customized with a {@link | ||
* HttpRouteHolder}, then this method will update the route using the provided {@link | ||
* HttpRouteGetter} if and only if the last {@link HttpRouteSource} to update the route using this | ||
* method has strictly lower priority than the provided {@link HttpRouteSource}, and the value | ||
* returned from the {@link HttpRouteGetter} is non-null. | ||
* | ||
* <p>If there is a server span in the context, and the context has NOT been customized with a | ||
* {@link HttpRouteHolder}, then this method will update the route using the provided {@link | ||
* HttpRouteGetter} if the value returned from it is non-null. | ||
*/ | ||
public static <T> void updateHttpRoute( | ||
Context context, HttpRouteSource source, HttpRouteGetter<T> httpRouteGetter, T arg1) { | ||
updateHttpRoute(context, source, OneArgAdapter.getInstance(), arg1, httpRouteGetter); | ||
} | ||
|
||
/** | ||
* Updates the {@code http.route} attribute in the received {@code context}. | ||
* | ||
* <p>If there is a server span in the context, and the context has been customized with a {@link | ||
* HttpRouteHolder}, then this method will update the route using the provided {@link | ||
* HttpRouteGetter2} if and only if the last {@link HttpRouteSource} to update the route using | ||
* this method has strictly lower priority than the provided {@link HttpRouteSource}, and the | ||
* value returned from the {@link HttpRouteGetter2} is non-null. | ||
* | ||
* <p>If there is a server span in the context, and the context has NOT been customized with a | ||
* {@code ServerSpanName}, then this method will update the route using the provided {@link | ||
* HttpRouteGetter2} if the value returned from it is non-null. | ||
*/ | ||
public static <T, U> void updateHttpRoute( | ||
Context context, | ||
HttpRouteSource source, | ||
HttpRouteGetter2<T, U> httpRouteGetter, | ||
T arg1, | ||
U arg2) { | ||
Span serverSpan = ServerSpan.fromContextOrNull(context); | ||
// checking isRecording() is a helpful optimization for more expensive suppliers | ||
// (e.g. Spring MVC instrumentation's HandlerAdapterInstrumentation) | ||
if (serverSpan == null || !serverSpan.isRecording()) { | ||
return; | ||
} | ||
HttpRouteHolder httpRouteHolder = context.get(CONTEXT_KEY); | ||
if (httpRouteHolder == null) { | ||
String httpRoute = httpRouteGetter.get(context, arg1, arg2); | ||
if (httpRoute != null && !httpRoute.isEmpty()) { | ||
updateSpanData(serverSpan, httpRoute); | ||
} | ||
return; | ||
} | ||
// special case for servlet filters, even when we have a route from previous filter see whether | ||
// the new route is better and if so use it instead | ||
boolean onlyIfBetterRoute = | ||
!source.useFirst && source.order == httpRouteHolder.updatedBySourceOrder; | ||
if (source.order > httpRouteHolder.updatedBySourceOrder || onlyIfBetterRoute) { | ||
String route = httpRouteGetter.get(context, arg1, arg2); | ||
if (route != null | ||
&& !route.isEmpty() | ||
&& (!onlyIfBetterRoute || httpRouteHolder.isBetterRoute(route))) { | ||
updateSpanData(serverSpan, route); | ||
httpRouteHolder.updatedBySourceOrder = source.order; | ||
httpRouteHolder.route = route; | ||
} | ||
} | ||
} | ||
|
||
// TODO: instead of calling setAttribute() consider storing the route in context end retrieving it | ||
// in the AttributesExtractor | ||
private static void updateSpanData(Span serverSpan, String route) { | ||
serverSpan.updateName(route); | ||
serverSpan.setAttribute(SemanticAttributes.HTTP_ROUTE, route); | ||
} | ||
|
||
// This is used when setting route from a servlet filter to pick the most descriptive (longest) | ||
// route. | ||
private boolean isBetterRoute(String name) { | ||
String route = this.route; | ||
int routeLength = route == null ? 0 : route.length(); | ||
return name.length() > routeLength; | ||
} | ||
|
||
// TODO: use that in HttpServerMetrics | ||
/** | ||
* Returns the {@code http.route} attribute value that's stored in the passed {@code context}, or | ||
* null if it was not set before. | ||
*/ | ||
@Nullable | ||
public static String getRoute(Context context) { | ||
HttpRouteHolder httpRouteHolder = context.get(CONTEXT_KEY); | ||
return httpRouteHolder == null ? null : httpRouteHolder.route; | ||
} | ||
|
||
private static final class OneArgAdapter<T> implements HttpRouteGetter2<T, HttpRouteGetter<T>> { | ||
|
||
private static final OneArgAdapter<Object> INSTANCE = new OneArgAdapter<>(); | ||
|
||
@SuppressWarnings("unchecked") | ||
static <T> OneArgAdapter<T> getInstance() { | ||
return (OneArgAdapter<T>) INSTANCE; | ||
} | ||
|
||
@Override | ||
@Nullable | ||
public String get(Context context, T arg, HttpRouteGetter<T> httpRouteGetter) { | ||
return httpRouteGetter.get(context, arg); | ||
} | ||
} | ||
} |
30 changes: 30 additions & 0 deletions
30
...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteSource.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,30 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.api.instrumenter.http; | ||
|
||
/** Represents the source that provided the {@code http.route} attribute. */ | ||
public enum HttpRouteSource { | ||
// for servlet filters we try to find the best name which isn't necessarily from the first | ||
// filter that is called | ||
FILTER(1, /* useFirst= */ false), | ||
SERVLET(2), | ||
CONTROLLER(3), | ||
// Some frameworks, e.g. JaxRS, allow for nested controller/paths and we want to select the | ||
// longest one | ||
NESTED_CONTROLLER(4, false); | ||
|
||
final int order; | ||
final boolean useFirst; | ||
|
||
HttpRouteSource(int order) { | ||
this(order, /* useFirst= */ true); | ||
} | ||
|
||
HttpRouteSource(int order, boolean useFirst) { | ||
this.order = order; | ||
this.useFirst = useFirst; | ||
} | ||
} |
16 changes: 0 additions & 16 deletions
16
...api/src/main/java/io/opentelemetry/instrumentation/api/server/ServerSpanNameSupplier.java
This file was deleted.
Oops, something went wrong.
16 changes: 0 additions & 16 deletions
16
...c/main/java/io/opentelemetry/instrumentation/api/server/ServerSpanNameTwoArgSupplier.java
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
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.
It's not obvious to me that it's better but as a reminder, the JDK idiom is to use
Bi
, not2
, e.g.BiFunction
BiConsumer
. I guess here our naming is constrained by having a fixedContext
param along with the arbitrary onesThere 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.
That being said, I guess having these types is to avoid capturing lambdas, but are we sure the performance improvement is really enough to avoid the simpler
Supplier<String>
orFunction<Context, String>
? With inlining and such it's always hard for me to really understand how bad a capturing lambda isThere 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.
Changed it
HttpRouteBiGetter
(because ofToDoubleBiFunction
in the JDK)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 opened #5215 (targeted to Stable API project) to make sure that this is an optimization worth keeping