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.

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

Refine 404 handling in Restlet instrumentation #4206

Merged
merged 2 commits into from
Sep 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package io.opentelemetry.javaagent.instrumentation.restlet.v1_0;

import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER;
import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext;
import static io.opentelemetry.javaagent.instrumentation.restlet.v1_0.RestletSingletons.instrumenter;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
Expand All @@ -13,13 +14,16 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
import io.opentelemetry.instrumentation.restlet.v1_0.internal.RestletServerSpanNaming;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;
import org.restlet.data.Request;
import org.restlet.data.Response;
import org.restlet.data.Status;

public class ServerInstrumentation implements TypeInstrumentation {
@Override
Expand Down Expand Up @@ -71,6 +75,11 @@ public static void finishRequest(

scope.close();

if (Status.CLIENT_ERROR_NOT_FOUND.equals(response.getStatus())) {
ServerSpanNaming.updateServerSpanName(
context, CONTROLLER, RestletServerSpanNaming.SERVER_SPAN_NAME, "/*");
}

if (exception != null) {
instrumenter().end(context, request, response, exception);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ import org.restlet.Component
import org.restlet.Context
import org.restlet.Redirector
import org.restlet.Restlet
import org.restlet.Router
import org.restlet.Server
import org.restlet.VirtualHost
import org.restlet.data.MediaType
import org.restlet.data.Protocol
import org.restlet.data.Request
import org.restlet.data.Response
import org.restlet.data.Status
import org.restlet.util.Template

import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
Expand All @@ -42,6 +44,7 @@ abstract class AbstractRestletServerTest extends HttpServerTest<Server> {

host = component.getDefaultHost()
attachRestlets()

component.start()

return server
Expand All @@ -52,11 +55,15 @@ abstract class AbstractRestletServerTest extends HttpServerTest<Server> {
component.stop()
}

def attachAndWrap(path, restlet){
def attachAndWrap(path, restlet) {
host.attach(path, wrapRestlet(restlet, path))
}

def attachRestlets(){
def attachRestlets() {

def defaultRouter = wrapRestlet(new Router(host.getContext()), "/*")
host.attach("/", defaultRouter).setMatchingMode(Template.MODE_STARTS_WITH)

attachAndWrap(SUCCESS.path, new Restlet() {
@Override
void handle(Request request, Response response) {
Expand All @@ -69,52 +76,42 @@ abstract class AbstractRestletServerTest extends HttpServerTest<Server> {

attachAndWrap(REDIRECT.path, new Redirector(Context.getCurrent(), REDIRECT.body, Redirector.MODE_CLIENT_FOUND) {
@Override
void handle(Request request, Response response){
void handle(Request request, Response response) {
super.handle(request, response)
controller(REDIRECT){
controller(REDIRECT) {
} //TODO: check why handle fails inside controller
}
})

attachAndWrap(ERROR.path, new Restlet(){
attachAndWrap(ERROR.path, new Restlet() {
@Override
void handle(Request request, Response response){
controller(ERROR){
void handle(Request request, Response response) {
controller(ERROR) {
response.setStatus(Status.valueOf(ERROR.getStatus()), ERROR.getBody())
}
}
})

attachAndWrap(EXCEPTION.path, new Restlet(){
attachAndWrap(EXCEPTION.path, new Restlet() {
@Override
void handle(Request request, Response response){
controller(EXCEPTION){
void handle(Request request, Response response) {
controller(EXCEPTION) {
throw new Exception(EXCEPTION.getBody())
}
}
})

attachAndWrap(QUERY_PARAM.path, new Restlet() {
@Override
void handle(Request request, Response response){
controller(QUERY_PARAM){
void handle(Request request, Response response) {
controller(QUERY_PARAM) {
response.setEntity(QUERY_PARAM.getBody(), MediaType.TEXT_PLAIN)
response.setStatus(Status.valueOf(QUERY_PARAM.getStatus()), QUERY_PARAM.getBody())
}
}
})

attachAndWrap(NOT_FOUND.path, new Restlet() {
@Override
void handle(Request request, Response response){
controller(NOT_FOUND){
response.setEntity(NOT_FOUND.getBody(), MediaType.TEXT_PLAIN)
response.setStatus(Status.valueOf(NOT_FOUND.getStatus()), NOT_FOUND.getBody())
}
}
})

attachAndWrap("/path/{id}/param", new Restlet(){
attachAndWrap("/path/{id}/param", new Restlet() {
@Override
void handle(Request request, Response response) {
controller(PATH_PARAM) {
Expand All @@ -128,7 +125,7 @@ abstract class AbstractRestletServerTest extends HttpServerTest<Server> {
@Override
void handle(Request request, Response response) {
controller(INDEXED_CHILD) {
INDEXED_CHILD.collectSpanAttributes {request.getOriginalRef().getQueryAsForm().getFirst(it).getValue() }
INDEXED_CHILD.collectSpanAttributes { request.getOriginalRef().getQueryAsForm().getFirst(it).getValue() }
response.setStatus(Status.valueOf(INDEXED_CHILD.status))
}
}
Expand All @@ -142,7 +139,7 @@ abstract class AbstractRestletServerTest extends HttpServerTest<Server> {
SemanticAttributes.HTTP_TARGET,
SemanticAttributes.HTTP_SCHEME,
SemanticAttributes.NET_TRANSPORT,
]
]
}

@Override
Expand All @@ -161,7 +158,7 @@ abstract class AbstractRestletServerTest extends HttpServerTest<Server> {
case PATH_PARAM:
return getContextPath() + "/path/{id}/param"
case NOT_FOUND:
return getContextPath() + "/notFound"
return getContextPath() + "/*"
default:
return endpoint.resolvePath(address).path
}
Expand Down