Skip to content

fix : added fallback support for host headers #279

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

Merged
merged 7 commits into from
Nov 3, 2021
Merged

fix : added fallback support for host headers #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a500595
added gethost method
Nov 2, 2021
c8b449e
modified
Nov 2, 2021
ae7c247
added unit test
Nov 2, 2021
f1a17aa
resolved PR comments
Nov 3, 2021
d2f3772
added unit test
Nov 3, 2021
998a063
nit
Nov 3, 2021
ddf8d24
nit
Nov 3, 2021
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
11 changes: 9 additions & 2 deletions ...a/org/hypertrace/traceenricher/enrichment/enrichers/ApiBoundaryTypeAttributeEnricher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public void enrichEvent(StructuredTrace trace, Event event) {
private void enrichHostHeader(Event event) {
Protocol protocol = EnrichedSpanUtils.getProtocol(event);
if (protocol == Protocol.PROTOCOL_GRPC) {
Optional<String> host = getGrpcAuthority(event);
Optional<String> host = getGrpcHostHeader(event);
if (host.isPresent()) {
addEnrichedAttribute(event, HOST_HEADER, AttributeValueCreator.create(host.get()));
return;
Expand All @@ -144,11 +144,18 @@ private void enrichHostHeader(Event event) {
}
}

private Optional<String> getGrpcAuthority(Event event) {
private Optional<String> getGrpcHostHeader(Event event) {
Copy link
Member Author

@sarthak77 sarthak77 Nov 2, 2021

Choose a reason for hiding this comment

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

@kotharironak should I add test over here also?

Copy link
Contributor

@kotharironak kotharironak Nov 3, 2021

Choose a reason for hiding this comment

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

Yes, add a test with an event not having authority but required tags like rpc.sytem and rec.request.metadata.host, and hostHeader fall back to metadata.host value.

Copy link
Contributor

@kotharironak kotharironak Nov 3, 2021

Choose a reason for hiding this comment

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

Refer the test file ApiBoundaryTypeAttributeEnricherTest, test cases testEnrichEventWithGrpcNoAuthority and similar.

Copy link
Member Author

@sarthak77 sarthak77 Nov 3, 2021

Choose a reason for hiding this comment

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

resolved

Copy link
Contributor

@kotharironak kotharironak Nov 3, 2021

Choose a reason for hiding this comment

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

There is a test testEnrichEventWithGrpcAuthority in the same file, can you also add the below attribute to it.

addAttributeToEvent(
        innerEntrySpan,
        RPC_REQUEST_METADATA_HOST.getValue(),
        AttributeValue.newBuilder().setValue("testHost2").build());

Copy link
Member Author

@sarthak77 sarthak77 Nov 3, 2021

Choose a reason for hiding this comment

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

done

Optional<String> grpcAuthority = RpcSemanticConventionUtils.getGrpcAuthority(event);
if (grpcAuthority.isPresent()) {
return getSanitizedHostValue(grpcAuthority.get());
}

Optional<String> grpcRequestMetadataHost =
RpcSemanticConventionUtils.getGrpcRequestMetadataHost(event);
if (grpcRequestMetadataHost.isPresent()) {
return getSanitizedHostValue(grpcRequestMetadataHost.get());
}

return Optional.empty();
}

Expand Down
24 changes: 24 additions & 0 deletions ...g/hypertrace/traceenricher/enrichment/enrichers/ApiBoundaryTypeAttributeEnricherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static org.hypertrace.core.span.normalizer.constants.OTelSpanTag.OTEL_SPAN_TAG_RPC_SYSTEM;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_AUTHORITY;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_HOST;
import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -273,6 +274,10 @@ public void testEnrichEventWithGrpcAuthority() {
innerEntrySpan,
RPC_REQUEST_METADATA_AUTHORITY.getValue(),
AttributeValue.newBuilder().setValue("testHost").build());
addAttributeToEvent(
innerEntrySpan,
RPC_REQUEST_METADATA_HOST.getValue(),
AttributeValue.newBuilder().setValue("testHost2").build());
addAttributeToEvent(
innerEntrySpan,
OTEL_SPAN_TAG_RPC_SYSTEM.getValue(),
Expand Down Expand Up @@ -300,6 +305,25 @@ public void testEnrichEventWithGrpcNoAuthority() {
Assertions.assertEquals(EnrichedSpanUtils.getHostHeader(innerEntrySpan), "testHost");
}

@Test
public void testEnrichEventWithGrpcNoAuthorityButRequestMetadataHost() {
mockProtocol(innerEntrySpan, Protocol.PROTOCOL_GRPC);
addEnrichedAttributeToEvent(
innerEntrySpan, X_FORWARDED_HOST_METADATA, AttributeValueCreator.create("testHost"));

addAttributeToEvent(
innerEntrySpan,
RPC_REQUEST_METADATA_HOST.getValue(),
AttributeValue.newBuilder().setValue("testHost2").build());
addAttributeToEvent(
innerEntrySpan,
OTEL_SPAN_TAG_RPC_SYSTEM.getValue(),
AttributeValue.newBuilder().setValue("grpc").build());

target.enrichEvent(trace, innerEntrySpan);
Assertions.assertEquals(EnrichedSpanUtils.getHostHeader(innerEntrySpan), "testHost2");
}

private void mockStructuredGraph() {
when(graph.getParentEvent(innerExitSpan)).thenReturn(innerEntrySpan);
when(graph.getChildrenEvents(innerExitSpan)).thenReturn(null);
Expand Down
16 changes: 16 additions & 0 deletions ...rc/main/java/org/hypertrace/semantic/convention/utils/rpc/RpcSemanticConventionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_BODY_TRUNCATED;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_AUTHORITY;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_CONTENT_LENGTH;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_HOST;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_PATH;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_USER_AGENT;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_X_FORWARDED_FOR;
Expand Down Expand Up @@ -68,6 +69,7 @@ public class RpcSemanticConventionUtils {
private static final String OTEL_RPC_SYSTEM_GRPC =
OTelRpcSemanticConventions.RPC_SYSTEM_VALUE_GRPC.getValue();
private static final String OTEL_SPAN_TAG_RPC_SYSTEM_ATTR = OTEL_SPAN_TAG_RPC_SYSTEM.getValue();
private static final String RPC_REQUEST_METADATA_HOST_ATTR = RPC_REQUEST_METADATA_HOST.getValue();

private static final String OTHER_GRPC_HOST_PORT = RawSpanConstants.getValue(Grpc.GRPC_HOST_PORT);
private static final String OTHER_GRPC_METHOD = RawSpanConstants.getValue(Grpc.GRPC_METHOD);
Expand Down Expand Up @@ -430,6 +432,20 @@ public static Optional<String> getGrpcXForwardedFor(Event event) {
return Optional.empty();
}

public static Optional<String> getGrpcRequestMetadataHost(Event event) {
if (event.getAttributes() == null || event.getAttributes().getAttributeMap() == null) {
return Optional.empty();
}

Map<String, AttributeValue> attributeValueMap = event.getAttributes().getAttributeMap();

if (!isRpcSystemGrpc(attributeValueMap)) {
return Optional.empty();
}
return Optional.ofNullable(attributeValueMap.get(RPC_REQUEST_METADATA_HOST_ATTR))
.map(AttributeValue::getValue);
}

public static Optional<String> getRpcPath(Event event) {
String service = getRpcService(event).orElse("");
String method = getRpcMethod(event).orElse("");
Expand Down
32 changes: 32 additions & 0 deletions ...est/java/org/hypertrace/semantic/convention/utils/rpc/RpcSemanticConventionUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_BODY_TRUNCATED;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_AUTHORITY;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_CONTENT_LENGTH;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_HOST;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_PATH;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_USER_AGENT;
import static org.hypertrace.core.span.normalizer.constants.RpcSpanTag.RPC_REQUEST_METADATA_X_FORWARDED_FOR;
Expand Down Expand Up @@ -165,6 +166,37 @@ public void testGetGrpcXForwardedFor() {
assertEquals("198.12.34.1", RpcSemanticConventionUtils.getGrpcXForwardedFor(event).get());
}

@Test
public void testGetGrpcRequestMetadataHostForGrpcSystem() {
Event event = mock(Event.class);
Copy link
Contributor

@kotharironak kotharironak Nov 3, 2021

Choose a reason for hiding this comment

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

Also add one more test, if rpc system is not set to Grpc, the method returns to Optional.Empty()

Copy link
Member Author

@sarthak77 sarthak77 Nov 3, 2021

Choose a reason for hiding this comment

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

resolved

when(event.getAttributes())
.thenReturn(
Attributes.newBuilder()
.setAttributeMap(
Map.of(
OTEL_SPAN_TAG_RPC_SYSTEM.getValue(),
AttributeValue.newBuilder().setValue("grpc").build(),
RPC_REQUEST_METADATA_HOST.getValue(),
AttributeValue.newBuilder().setValue("webhost:9011").build()))
.build());
assertEquals(
"webhost:9011", RpcSemanticConventionUtils.getGrpcRequestMetadataHost(event).get());
}

@Test
public void testGetGrpcRequestMetadataHostForNotGrpcSystem() {
Event event = mock(Event.class);
when(event.getAttributes())
.thenReturn(
Attributes.newBuilder()
.setAttributeMap(
Map.of(
RPC_REQUEST_METADATA_HOST.getValue(),
AttributeValue.newBuilder().setValue("webhost:9011").build()))
.build());
assertEquals(Optional.empty(), RpcSemanticConventionUtils.getGrpcRequestMetadataHost(event));
}

@Test
public void testGetGrpcStatusMsg() {
Event event =
Expand Down
1 change: 1 addition & 0 deletions ...zer-constants/src/main/java/org/hypertrace/core/span/normalizer/constants/RpcSpanTag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ public enum RpcSpanTag {
RPC_ERROR_NAME("rpc.error_name"),
RPC_ERROR_MESSAGE("rpc.error_message"),
RPC_REQUEST_METADATA("rpc.request.metadata"),
RPC_REQUEST_METADATA_HOST("rpc.request.metadata.host"),
RPC_RESPONSE_METADATA("rpc.response.metadata"),
RPC_REQUEST_BODY("rpc.request.body"),
RPC_RESPONSE_BODY("rpc.response.body"),
Expand Down