Skip to content

Commit

Permalink
Updates to http.server_name (open-telemetry#5369)
Browse files Browse the repository at this point in the history
* Updates to http.server_name

* Tests

* fix

* armeria

* fix

* fix

* codenarc
  • Loading branch information
trask authored and RashmiRam committed May 23, 2022
1 parent 555a32f commit 079c9fe
Show file tree
Hide file tree
Showing 39 changed files with 31 additions and 192 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
set(attributes, SemanticAttributes.HTTP_HOST, host(request));
set(attributes, SemanticAttributes.HTTP_TARGET, getter.target(request));
set(attributes, SemanticAttributes.HTTP_ROUTE, getter.route(request));
set(attributes, SemanticAttributes.HTTP_SERVER_NAME, getter.serverName(request));
set(attributes, SemanticAttributes.HTTP_CLIENT_IP, clientIp(request));
}

Expand All @@ -80,7 +81,6 @@ public void onEnd(
@Nullable Throwable error) {

super.onEnd(attributes, context, request, response, error);
set(attributes, SemanticAttributes.HTTP_SERVER_NAME, getter.serverName(request, response));
set(attributes, SemanticAttributes.HTTP_ROUTE, httpRouteHolderGetter.apply(context));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.instrumentation.api.instrumenter.http;

import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import javax.annotation.Nullable;

/**
Expand All @@ -33,14 +31,11 @@ public interface HttpServerAttributesGetter<REQUEST, RESPONSE>
@Nullable
String scheme(REQUEST request);

// Attributes which are not always available when the request is ready.

/**
* Extracts the {@code http.server_name} span attribute.
*
* <p>This is called from {@link Instrumenter#end(Context, Object, Object, Throwable)}, whether
* {@code response} is {@code null} or not.
* The primary server name of the matched virtual host. This should be obtained via configuration,
* not from the Host header. If no such configuration can be obtained, this method should return
* {@code null}.
*/
@Nullable
String serverName(REQUEST request, @Nullable RESPONSE response);
String serverName(REQUEST request);
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public String scheme(Map<String, String> request) {
}

@Override
public String serverName(Map<String, String> request, Map<String, String> response) {
public String serverName(Map<String, String> request) {
return request.get("serverName");
}

Expand Down Expand Up @@ -146,6 +146,7 @@ void normal() {
entry(SemanticAttributes.HTTP_TARGET, "/repositories/1"),
entry(SemanticAttributes.HTTP_USER_AGENT, "okhttp 3.x"),
entry(SemanticAttributes.HTTP_ROUTE, "/repositories/{id}"),
entry(SemanticAttributes.HTTP_SERVER_NAME, "server"),
entry(SemanticAttributes.HTTP_CLIENT_IP, "1.1.1.1"),
entry(
AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
Expand All @@ -160,6 +161,7 @@ void normal() {
entry(SemanticAttributes.HTTP_TARGET, "/repositories/1"),
entry(SemanticAttributes.HTTP_USER_AGENT, "okhttp 3.x"),
entry(SemanticAttributes.HTTP_ROUTE, "/repositories/{repoId}"),
entry(SemanticAttributes.HTTP_SERVER_NAME, "server"),
entry(SemanticAttributes.HTTP_CLIENT_IP, "1.1.1.1"),
entry(
AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public String scheme(HttpRequest request) {

@Override
@Nullable
public String serverName(HttpRequest request, @Nullable HttpResponse httpResponse) {
public String serverName(HttpRequest request) {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ class RestCamelTest extends AgentInstrumentationSpecification implements RetryOn
"$SemanticAttributes.HTTP_METHOD" "GET"
"$SemanticAttributes.NET_PEER_IP" "127.0.0.1"
"$SemanticAttributes.NET_PEER_PORT" Long
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" "/api/{module}/unit/{unitId}"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ class TwoServicesWithDirectClientCamelTest extends AgentInstrumentationSpecifica
"$SemanticAttributes.NET_PEER_IP" "127.0.0.1"
"$SemanticAttributes.HTTP_USER_AGENT" "Jakarta Commons-HttpClient/3.1"
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH" Long
"$SemanticAttributes.HTTP_ROUTE" "/serviceTwo"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,9 @@ public List<String> responseHeader(RequestContext ctx, RequestLog requestLog, St

@Override
@Nullable
public String serverName(RequestContext ctx, @Nullable RequestLog requestLog) {
public String serverName(RequestContext ctx) {
if (ctx instanceof ServiceRequestContext) {
return ((ServiceRequestContext) ctx).config().virtualHost().hostnamePattern();
return ((ServiceRequestContext) ctx).config().virtualHost().defaultHostname();
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@ import io.dropwizard.setup.Bootstrap
import io.dropwizard.setup.Environment
import io.dropwizard.testing.ConfigOverride
import io.dropwizard.testing.DropwizardTestSupport
import io.opentelemetry.api.common.AttributeKey
import io.opentelemetry.api.trace.StatusCode
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.asserts.TraceAssert
import io.opentelemetry.instrumentation.test.base.HttpServerTest
import io.opentelemetry.instrumentation.test.utils.PortUtils
import io.opentelemetry.sdk.trace.data.SpanData
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes

import javax.ws.rs.GET
import javax.ws.rs.HeaderParam
Expand Down Expand Up @@ -62,14 +60,6 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport> implements Ag
testSupport.after()
}

@Override
Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
Set<AttributeKey<?>> extra = [
SemanticAttributes.HTTP_SERVER_NAME
]
super.httpAttributes(endpoint) + extra
}

// this override is needed because dropwizard reports peer ip as the client ip
@Override
String peerIp(ServerEndpoint endpoint) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,11 @@ package test
import grails.boot.GrailsApp
import grails.boot.config.GrailsAutoConfiguration
import groovy.transform.CompileStatic
import io.opentelemetry.api.common.AttributeKey
import io.opentelemetry.api.trace.StatusCode
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.asserts.TraceAssert
import io.opentelemetry.instrumentation.test.base.HttpServerTest
import io.opentelemetry.sdk.trace.data.SpanData
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
import org.springframework.boot.autoconfigure.SpringBootApplication
import org.springframework.boot.autoconfigure.web.ServerProperties
import org.springframework.context.ConfigurableApplicationContext
Expand Down Expand Up @@ -56,14 +54,6 @@ class GrailsTest extends HttpServerTest<ConfigurableApplicationContext> implemen
}
}

@Override
Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
Set<AttributeKey<?>> extra = [
SemanticAttributes.HTTP_SERVER_NAME
]
super.httpAttributes(endpoint) + extra
}

@Override
String expectedHttpRoute(ServerEndpoint endpoint) {
if (endpoint == PATH_PARAM) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public String scheme(HttpRequestPacket request) {

@Nullable
@Override
public String serverName(HttpRequestPacket request, @Nullable HttpResponsePacket response) {
public String serverName(HttpRequestPacket request) {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* SPDX-License-Identifier: Apache-2.0
*/

import io.opentelemetry.api.common.AttributeKey
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.asserts.TraceAssert
import io.opentelemetry.instrumentation.test.base.HttpServerTest
Expand Down Expand Up @@ -189,14 +188,6 @@ abstract class JaxRsHttpServerTest<S> extends HttpServerTest<S> implements Agent
"failing" | "throw" | 500 | { it == "failure" } | true | "failure"
}
@Override
Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
Set<AttributeKey<?>> extra = [
SemanticAttributes.HTTP_SERVER_NAME
]
super.httpAttributes(endpoint) + extra
}
@Override
boolean hasHandlerSpan(ServerEndpoint endpoint) {
true
Expand Down Expand Up @@ -283,7 +274,6 @@ abstract class JaxRsHttpServerTest<S> extends HttpServerTest<S> implements Agent
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" TEST_USER_AGENT
"$SemanticAttributes.HTTP_CLIENT_IP" TEST_CLIENT_IP
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
// Optional
"$SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH" { it == null || it instanceof Long }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ class JettyHandlerTest extends HttpServerTest<Server> implements AgentTestTrait
Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
def attributes = super.httpAttributes(endpoint)
attributes.remove(SemanticAttributes.HTTP_ROUTE)
attributes.add(SemanticAttributes.HTTP_SERVER_NAME)
attributes
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ class JettyHandlerTest extends HttpServerTest<Server> implements AgentTestTrait
Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
def attributes = super.httpAttributes(endpoint)
attributes.remove(SemanticAttributes.HTTP_ROUTE)
attributes.addAll(SemanticAttributes.HTTP_SERVER_NAME)
attributes
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
// Optional
"$SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH" { it == null || it instanceof Long }
Expand Down Expand Up @@ -158,7 +157,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -211,7 +209,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH" Long
"$SemanticAttributes.HTTP_ROUTE" route
Expand Down Expand Up @@ -274,7 +271,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 500
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -341,7 +337,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -389,7 +384,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -469,7 +463,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 500
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -518,7 +511,6 @@ class JspInstrumentationBasicTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -166,7 +165,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -214,7 +212,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -310,7 +307,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 200
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -392,7 +388,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 500
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down Expand Up @@ -453,7 +448,6 @@ class JspInstrumentationForwardTests extends AgentInstrumentationSpecification {
"$SemanticAttributes.HTTP_STATUS_CODE" 404
"$SemanticAttributes.HTTP_FLAVOR" "1.1"
"$SemanticAttributes.HTTP_USER_AGENT" String
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" route
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ internal class KtorHttpServerAttributesGetter :
return request.origin.scheme
}

override fun serverName(request: ApplicationRequest, response: ApplicationResponse?): String? {
override fun serverName(request: ApplicationRequest): String? {
return null
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,7 @@ public String route(LibertyRequest libertyRequest) {

@Override
@Nullable
public String serverName(
LibertyRequest libertyRequest, @Nullable LibertyResponse libertyResponse) {
public String serverName(LibertyRequest libertyRequest) {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,7 @@ public String scheme(HttpRequestAndChannel requestAndChannel) {

@Override
@Nullable
public String serverName(
HttpRequestAndChannel requestAndChannel, @Nullable HttpResponse response) {
public String serverName(HttpRequestAndChannel requestAndChannel) {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,7 @@ public String scheme(HttpRequestAndChannel requestAndChannel) {

@Override
@Nullable
public String serverName(
HttpRequestAndChannel requestAndChannel, @Nullable HttpResponse response) {
public String serverName(HttpRequestAndChannel requestAndChannel) {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public String flavor(Request request) {

@Override
@Nullable
public String serverName(Request request, @Nullable Response response) {
public String serverName(Request request) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public String flavor(Request request) {

@Override
@Nullable
public String serverName(Request request, @Nullable Response response) {
public String serverName(Request request) {
return null;
}

Expand Down
Loading

0 comments on commit 079c9fe

Please sign in to comment.