Skip to content

Fallback to db.connection_string for sql backend url #84

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 5 commits into from
Dec 14, 2020
Merged

Fallback to db.connection_string for sql backend url #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f5ec91
Use db.connection_string for sql backend url
Dec 7, 2020
9974ad5
Merge branch 'main' into sql-url-fallback
Dec 14, 2020
5237866
Add test
Dec 14, 2020
e254413
Remove unusued imports
Dec 14, 2020
d53590b
Check if the db connection string is a valid uri
Dec 14, 2020
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
45 changes: 44 additions & 1 deletion .../src/main/java/org/hypertrace/semantic/convention/utils/db/DbSemanticConventionUtils.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.collect.Lists;
import com.google.common.collect.Sets;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -163,7 +165,39 @@ public static Optional<String> getSqlURI(Event event) {
if (SpanAttributeUtils.containsAttributeKey(event, SQL_URL)) {
return Optional.of(SpanAttributeUtils.getStringAttribute(event, SQL_URL));
}
return getBackendURIForOtelFormat(event);
Optional<String> backendUrl = getBackendURIForOtelFormat(event);
if (backendUrl.isPresent()) {
return backendUrl;
}
if (SpanAttributeUtils.containsAttributeKey(
event, OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue())) {
String url = SpanAttributeUtils.getStringAttribute(
Copy link
Contributor

@kotharironak kotharironak Dec 14, 2020

Choose a reason for hiding this comment

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

Don't we have to convert this String to URI object?

try {
new URI(URL)
} catch (Mulformed) {
}

Copy link
Contributor Author

@findingrish findingrish Dec 14, 2020

Choose a reason for hiding this comment

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

done

event, OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue());
if (!isValidURI(url)) {
Copy link
Contributor

@jcchavezs jcchavezs Dec 14, 2020

Choose a reason for hiding this comment

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

nit, if you do isValirURI(url) { return Optional.of(url); } I think you can just keep the last return Optional.empty().

Copy link
Contributor Author

@findingrish findingrish Dec 14, 2020

Choose a reason for hiding this comment

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

Sure, I will do it in a subsequent pr.

return Optional.empty();
}
return Optional.of(url);
}
return Optional.empty();
}

public static Optional<String> getSqlUrlForOtelFormat(
Map<String, AttributeValue> attributeValueMap) {
if (!isSqlTypeBackendForOtelFormat(attributeValueMap)) {
return Optional.empty();
}
Optional<String> backendUrl = getBackendURIForOtelFormat(attributeValueMap);
if (backendUrl.isPresent()) {
return backendUrl;
}
if (attributeValueMap.containsKey(OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue())) {
String url = attributeValueMap.get(OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue()).getValue();
if (!isValidURI(url)) {
return Optional.empty();
}
return Optional.of(url);
}
return Optional.empty();
}

/**
Expand Down Expand Up @@ -192,4 +226,13 @@ public static Optional<String> getDbTypeForOtelFormat(Event event) {
}
return Optional.empty();
}

static boolean isValidURI(String uri) {
try {
new URI(uri);
} catch (URISyntaxException e) {
return false;
}
return true;
}
}
41 changes: 41 additions & 0 deletions .../test/java/org/hypertrace/semantic/convention/utils/db/DbSemanticConventionUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import java.util.Map;
import java.util.Optional;
import org.hypertrace.core.datamodel.AttributeValue;
import org.hypertrace.core.semantic.convention.constants.db.OTelDbSemanticConventions;
import org.hypertrace.core.semantic.convention.constants.span.OTelSpanSemanticConventions;
import org.hypertrace.semantic.convention.utils.SemanticConventionTestUtil;
Expand Down Expand Up @@ -193,6 +194,46 @@ public void testGetSqlURI() {
when(e.getAttributes()).thenReturn(attributes);
v = DbSemanticConventionUtils.getSqlURI(e);
assertTrue(v.isEmpty());

attributes = SemanticConventionTestUtil.buildAttributes(
Map.of(
OTelDbSemanticConventions.DB_SYSTEM.getValue(),
SemanticConventionTestUtil.buildAttributeValue(OTelDbSemanticConventions.MYSQL_DB_SYSTEM_VALUE.getValue()),
OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue(),
SemanticConventionTestUtil.buildAttributeValue("jdbc:mysql://mysql:3306/shop")));
when(e.getAttributes()).thenReturn(attributes);
v = DbSemanticConventionUtils.getSqlURI(e);
assertEquals("jdbc:mysql://mysql:3306/shop", v.get());
}

@Test
public void testGetSqlUrlForOtelFormat() {
Copy link
Contributor

@jcchavezs jcchavezs Dec 14, 2020

Choose a reason for hiding this comment

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

I wonder if we need some test where the user/password are included in the URL. Not sure tif that is a thing among jdbc.

Copy link
Contributor Author

@findingrish findingrish Dec 14, 2020

Choose a reason for hiding this comment

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

You mean if username/pwd is present we should drop the url?

Map<String, AttributeValue> map =
Map.of(
OTelDbSemanticConventions.DB_SYSTEM.getValue(),
SemanticConventionTestUtil.buildAttributeValue(OTelDbSemanticConventions.MYSQL_DB_SYSTEM_VALUE.getValue()),
OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue(),
SemanticConventionTestUtil.buildAttributeValue("jdbc:mysql://mysql:3306/shop"));
Optional<String> v = DbSemanticConventionUtils.getSqlUrlForOtelFormat(map);
assertEquals("jdbc:mysql://mysql:3306/shop", v.get());

map = Map.of(
OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue(),
SemanticConventionTestUtil.buildAttributeValue("jdbc:mysql://mysql:3306/shop")
);
v = DbSemanticConventionUtils.getSqlUrlForOtelFormat(map);
assertTrue(v.isEmpty());

map =
Map.of(
OTelSpanSemanticConventions.NET_PEER_IP.getValue(),
SemanticConventionTestUtil.buildAttributeValue("127.0.0.1"),
OTelDbSemanticConventions.DB_SYSTEM.getValue(),
SemanticConventionTestUtil.buildAttributeValue(OTelDbSemanticConventions.MYSQL_DB_SYSTEM_VALUE.getValue()),
OTelDbSemanticConventions.DB_CONNECTION_STRING.getValue(),
SemanticConventionTestUtil.buildAttributeValue("jdbc:mysql://mysql:3306/shop"));
v = DbSemanticConventionUtils.getSqlUrlForOtelFormat(map);
assertEquals("127.0.0.1", v.get());
}

@Test
Expand Down
6 changes: 2 additions & 4 deletions .../src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/SqlFieldsGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,7 @@ protected void populateOtherFields(Event.Builder eventBuilder, final Map<String,
protected void maybePopulateSqlUrlForOtelSpan(
Event.Builder eventBuilder,
final Map<String, AttributeValue> attributeFieldMap) {
if (DbSemanticConventionUtils.isSqlTypeBackendForOtelFormat(attributeFieldMap)) {
Optional<String> sqlUrl = DbSemanticConventionUtils.getBackendURIForOtelFormat(attributeFieldMap);
sqlUrl.ifPresent(s -> eventBuilder.getSqlBuilder().setUrl(s));
}
Optional<String> url = DbSemanticConventionUtils.getSqlUrlForOtelFormat(attributeFieldMap);
url.ifPresent(s -> eventBuilder.getSqlBuilder().setUrl(s));
}
}