Skip to content

Set Event.http.request.url on a best effort basis #99

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

Closed
wants to merge 3 commits into from
Closed

Set Event.http.request.url on a best effort basis #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6889bd8
Http.request.url is populated on a best effort basis, EnrichedSpanUti…
Jan 7, 2021
f1594b5
Merge branch 'main' into populate-http-url
Jan 8, 2021
c2c383c
Address review comments
Jan 8, 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
20 changes: 17 additions & 3 deletions ...ain/java/org/hypertrace/traceenricher/enrichedspan/constants/utils/EnrichedSpanUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import java.net.MalformedURLException;
import java.net.URL;
import java.nio.ByteBuffer;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -301,13 +303,25 @@ public static Optional<String> getHttpMethod(Event event) {
}

public static Optional<String> getFullHttpUrl(Event event) {
if (event.getHttp() != null && event.getHttp().getRequest() != null) {
return Optional.ofNullable(event.getHttp().getRequest().getUrl());
if (event.getHttp() != null
&& event.getHttp().getRequest() != null
&& event.getHttp().getRequest().getUrl() != null
&& isAbsoluteUrl(event.getHttp().getRequest().getUrl())) {
Copy link
Contributor

@buchi-busireddy buchi-busireddy Jan 8, 2021

Choose a reason for hiding this comment

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

when the URL isn't full URL, do you want to rather check if you can construct full URL back from scheme, authority, path and query params? That's what I had in mind.

Copy link
Contributor

@buchi-busireddy buchi-busireddy Jan 8, 2021

Choose a reason for hiding this comment

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

Oh.. if you're making sure this URL is always full URL, we don't even need this method I think.

Copy link
Contributor Author

@findingrish findingrish Jan 8, 2021

Choose a reason for hiding this comment

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

We would need it.
Earlier, event.getHttp().getRequest().getUrl() was null in case of relative url. Now since it is being populated in a best effort basis, so it maybe full url or not.
However the consumer of this method EnrichedSpanUtils#getFullHttpUrl would except a full url to be returned or null.
To ensure that behaviour a check has been added to this method.

However I think we can go with this pr first #104 for now. It is a subset of the changes made in this pr.

return Optional.of(event.getHttp().getRequest().getUrl());
}

return Optional.empty();
}

private static boolean isAbsoluteUrl(String urlStr) {
Copy link
Contributor

@skjindal93 skjindal93 Jan 7, 2021

Choose a reason for hiding this comment

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

Who is using the private function?

Copy link
Contributor Author

@findingrish findingrish Jan 8, 2021

Choose a reason for hiding this comment

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

This method getFullHttpUrl

try {
URL url = new URL(new URL("http://hypertrace.org"), urlStr);
Copy link
Contributor

@buchi-busireddy buchi-busireddy Jan 8, 2021

Choose a reason for hiding this comment

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

Why do we need this http://hypertrace.org?

Copy link
Contributor Author

@findingrish findingrish Jan 8, 2021

Choose a reason for hiding this comment

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

Any other url will be fine too. In case when input url is relative, this provided url makes up for the scheme, authority etc.

return url.toString().equals(urlStr);
} catch (MalformedURLException e) {
// ignore
}
return false;
}

public static Optional<Integer> getRequestSize(Event event) {
Protocol protocol = EnrichedSpanUtils.getProtocol(event);
if (protocol == null) {
Expand Down
7 changes: 6 additions & 1 deletion ...nerator/src/main/java/org/hypertrace/viewgenerator/generators/SpanEventViewGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import org.apache.avro.Schema;
import org.hypertrace.core.datamodel.Entity;
import org.hypertrace.core.datamodel.Event;
import org.hypertrace.core.datamodel.MetricValue;
import org.hypertrace.core.datamodel.StructuredTrace;
import org.hypertrace.core.datamodel.eventfields.http.Http;
import org.hypertrace.core.datamodel.eventfields.http.Request;
import org.hypertrace.core.datamodel.shared.SpanAttributeUtils;
import org.hypertrace.traceenricher.enrichedspan.constants.EnrichedSpanConstants;
import org.hypertrace.traceenricher.enrichedspan.constants.utils.EnrichedSpanUtils;
Expand Down Expand Up @@ -292,7 +295,9 @@ String getRequestUrl(Event event, Protocol protocol) {
switch (protocol) {
case PROTOCOL_HTTP:
case PROTOCOL_HTTPS:
return EnrichedSpanUtils.getFullHttpUrl(event).orElse(event.getHttp().getRequest().getPath());
return Optional.ofNullable(event.getHttp())
.map(Http::getRequest)
.map(Request::getUrl).orElse(null);
case PROTOCOL_GRPC:
return event.getEventName();
}
Expand Down
1 change: 1 addition & 0 deletions ...tor/src/test/java/org/hypertrace/viewgenerator/generators/SpanEventViewGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down
23 changes: 18 additions & 5 deletions ...src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ private static void setUserAgent(
.ifPresent(userAgent -> httpBuilder.getRequestBuilder().setUserAgent(userAgent));
}

private static void setPath(
static void setPath(
Http.Builder httpBuilder, Map<String, JaegerSpanInternalModel.KeyValue> tagsMap) {
if (httpBuilder.getRequestBuilder().hasPath()) {
return;
Expand All @@ -469,7 +469,7 @@ private static String removeTrailingSlash(String s) {
return s.endsWith(SLASH) && s.length() > 1 ? s.substring(0, s.length() - 1) : s;
}

private static Optional<String> getPathFromUrlObject(String urlPath) {
static Optional<String> getPathFromUrlObject(String urlPath) {
try {
URL url = getNormalizedUrl(urlPath);
return Optional.of(url.getPath());
Expand Down Expand Up @@ -552,8 +552,6 @@ private void populateUrlParts(Request.Builder requestBuilder) {
if (url.toString().equals(urlStr)) { // absolute URL
requestBuilder.setScheme(url.getProtocol());
requestBuilder.setHost(url.getAuthority()); // Use authority so in case the port is specified it adds it to this
} else { // relative URL
requestBuilder.setUrl(null); // unset the URL as we only allow absolute/full URLs in the url field
}
setPathFromUrl(requestBuilder, url);
if (!requestBuilder.hasQueryString()) {
Expand All @@ -569,13 +567,28 @@ private static URL getNormalizedUrl(String url) throws MalformedURLException {
return new URL(new URL(RELATIVE_URL_CONTEXT), url);
}

/**
* If the requestBuilder already has a url, which is absolute do nothing
* if not, try building a url based on otel attributes
*/
private void maybeSetHttpUrlForOtelFormat(
Request.Builder requestBuilder,
final Map<String, AttributeValue> attributeValueMap) {
if (requestBuilder.hasUrl()) {
if (requestBuilder.hasUrl() && isAbsoluteUrl(requestBuilder.getUrl())) {
return;
}
// if requestBuilder.getUrl() is not absolute try building the url from other attributes
Optional<String> url = HttpSemanticConventionUtils.getHttpUrlForOTelFormat(attributeValueMap);
url.ifPresent(requestBuilder::setUrl);
}

static boolean isAbsoluteUrl(String urlStr) {
Copy link
Contributor

@skjindal93 skjindal93 Jan 7, 2021

Choose a reason for hiding this comment

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

I believe it's the same method defined in EnrichedSpanUtils above.

Copy link
Contributor Author

@findingrish findingrish Jan 8, 2021

Choose a reason for hiding this comment

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

Yes it is, since span-normalizer doesn't depend on enriched-span-constants or vice-versa, I just duplicated the code.

try {
URL url = getNormalizedUrl(urlStr);
return url.toString().equals(urlStr);
} catch (MalformedURLException e) {
// ignore
}
return false;
}
}
48 changes: 44 additions & 4 deletions ...test/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import static org.hypertrace.core.span.constants.v1.OTSpanTag.OT_SPAN_TAG_HTTP_URL;
import static org.hypertrace.core.spannormalizer.utils.TestUtils.createKeyValue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import com.google.common.collect.Maps;
import io.jaegertracing.api_v2.JaegerSpanInternalModel;
Expand All @@ -46,9 +48,13 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.apache.http.client.methods.RequestBuilder;
import org.hypertrace.core.datamodel.AttributeValue;
import org.hypertrace.core.datamodel.Event;
import org.hypertrace.core.datamodel.eventfields.http.Http;
import org.hypertrace.core.datamodel.eventfields.http.Request;
import org.hypertrace.core.semantic.convention.constants.span.OTelSpanSemanticConventions;
import org.hypertrace.core.span.constants.RawSpanConstants;
import org.hypertrace.core.semantic.convention.constants.http.OTelHttpSemanticConventions;
import org.junit.jupiter.api.Assertions;
Expand Down Expand Up @@ -589,7 +595,7 @@ public void testRelativeUrlNotSetsUrlField() {
assertEquals("/dispatch/test?a=b&k1=v1", httpBuilder.getRequestBuilder().getUrl());

httpFieldsGenerator.populateOtherFields(eventBuilder, Maps.newHashMap()); // this should unset the url field
Assertions.assertNull(httpBuilder.getRequestBuilder().getUrl());
Assertions.assertEquals("/dispatch/test?a=b&k1=v1", httpBuilder.getRequestBuilder().getUrl());
}

@Test
Expand Down Expand Up @@ -696,7 +702,7 @@ public void testPopulateOtherFields() {
eventBuilder.getHttpBuilder().getRequestBuilder().setUrl("/apis/5673/events?a1=v1&a2=v2");
httpFieldsGenerator.populateOtherFields(eventBuilder, Maps.newHashMap());

Assertions.assertNull(eventBuilder.getHttpBuilder().getRequestBuilder().getUrl());
Assertions.assertEquals("/apis/5673/events?a1=v1&a2=v2", eventBuilder.getHttpBuilder().getRequestBuilder().getUrl());
Assertions.assertNull(eventBuilder.getHttpBuilder().getRequestBuilder().getScheme());
Assertions.assertNull(eventBuilder.getHttpBuilder().getRequestBuilder().getHost());
assertEquals("/apis/5673/events", eventBuilder.getHttpBuilder().getRequestBuilder().getPath());
Expand Down Expand Up @@ -836,10 +842,9 @@ public void testPopulateOtherFieldsOTelSpan() {
eventBuilder = Event.newBuilder();
eventBuilder.getHttpBuilder().getRequestBuilder().setUrl("/apis/5673/events?a1=v1&a2=v2");
map = Maps.newHashMap();
map.put(OTelHttpSemanticConventions.HTTP_URL.getValue(), buildAttributeValue(url));
httpFieldsGenerator.populateOtherFields(eventBuilder, map);

Assertions.assertNull(eventBuilder.getHttpBuilder().getRequestBuilder().getUrl());
Assertions.assertEquals("/apis/5673/events?a1=v1&a2=v2", eventBuilder.getHttpBuilder().getRequestBuilder().getUrl());
Assertions.assertNull(eventBuilder.getHttpBuilder().getRequestBuilder().getScheme());
Assertions.assertNull(eventBuilder.getHttpBuilder().getRequestBuilder().getHost());
assertEquals("/apis/5673/events", eventBuilder.getHttpBuilder().getRequestBuilder().getPath());
Expand Down Expand Up @@ -876,6 +881,41 @@ public void testPopulateOtherFieldsOTelSpan() {
"/some-test-path", eventBuilder.getHttpBuilder().getRequestBuilder().getPath());
assertEquals(
"some-query-str=v1", eventBuilder.getHttpBuilder().getRequestBuilder().getQueryString());

// originally set url is a relative url, should be overridden
eventBuilder = Event.newBuilder();
eventBuilder.getHttpBuilder().getRequestBuilder().setUrl("/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel");
map = Maps.newHashMap();
map.put(OTelHttpSemanticConventions.HTTP_SCHEME.getValue(), buildAttributeValue("http"));
map.put(OTelSpanSemanticConventions.SPAN_KIND.getValue(), buildAttributeValue("server"));
map.put(OTelHttpSemanticConventions.HTTP_NET_HOST_NAME.getValue(), buildAttributeValue("example.internal.com"));
map.put(OTelHttpSemanticConventions.HTTP_NET_HOST_PORT.getValue(), buildAttributeValue("50850"));
map.put(OTelHttpSemanticConventions.HTTP_TARGET.getValue(), buildAttributeValue("/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel"));
httpFieldsGenerator.populateOtherFields(eventBuilder, map);
assertEquals("http://example.internal.com:50850/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel", eventBuilder.getHttpBuilder().getRequestBuilder().getUrl());
}

@Test
public void testIsAbsoluteUrl() {
assertTrue(HttpFieldsGenerator.isAbsoluteUrl("http://example.com/abc/xyz"));
assertFalse(HttpFieldsGenerator.isAbsoluteUrl("/abc/xyz"));
}

@Test
public void testGetPathFromUrl() {
Optional<String> path = HttpFieldsGenerator.getPathFromUrlObject(
"/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel");
assertEquals(path.get(), "/api/v1/gatekeeper/check");
}

@Test
public void testSetPath() {
Map<String, JaegerSpanInternalModel.KeyValue> tagsMap = new HashMap<>();
tagsMap.put(OTelHttpSemanticConventions.HTTP_TARGET.getValue(),
createKeyValue("/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel"));
Http.Builder builder = Http.newBuilder().setRequestBuilder(Request.newBuilder());
HttpFieldsGenerator.setPath(builder, tagsMap);
assertEquals(builder.getRequestBuilder().getPath(), "/api/v1/gatekeeper/check");
}

private static AttributeValue buildAttributeValue(String value) {
Expand Down