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

Adds XRay specific brave codes #535

Closed
wants to merge 2 commits into from
Closed
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
@@ -0,0 +1,27 @@
package brave.http.aws;

import brave.SpanCustomizer;
import brave.http.HttpAdapter;
import brave.http.HttpServerParser;
import zipkin.TraceKeys;

public class XRayHttpClientParser extends HttpServerParser {

private boolean usePath = false;

public XRayHttpClientParser(boolean usePath) {
this.usePath = usePath;
}

public XRayHttpClientParser() {
this(false);
}

@Override
public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer customizer) {
String url = usePath ? adapter.path(req) : adapter.url(req);
if (url != null) customizer.tag(TraceKeys.HTTP_URL, url);
super.request(adapter, req, customizer);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package brave.http.aws;

import brave.SpanCustomizer;
import brave.http.HttpAdapter;
import brave.http.HttpServerParser;
import zipkin.TraceKeys;

public class XRayHttpServerParser extends HttpServerParser {
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll end up with something like XRayParser.httpServer() httpClient() sqlClient() etc. If we do our dependencies well, then we can have brave-parser-xray live in zipkin-aws and only depend on brave-instrumentation-http and a future brave-instrumentation-sql

Copy link
Author

Choose a reason for hiding this comment

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

@adriancole Can you expand this a little bid more? Since TracingStatementInterceptor are appended to sql connection strings, it made me confused.

Copy link
Member

Choose a reason for hiding this comment

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

I mean to create a generic parser for SQL databases, similar to what we have in brave-instrumentation-http. ex a new support module which would allow you to plug-in SqlClientParser like HttpClientParser. ex brave-instrumentation-sql

Copy link
Member

Choose a reason for hiding this comment

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

if you did this, it would decouple xray from each individual sql instrumentation library


private boolean usePath = false;

public XRayHttpServerParser(boolean usePath) {
this.usePath = usePath;
}

public XRayHttpServerParser() {
this(false);
}

@Override
public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer customizer) {
String url = usePath ? adapter.path(req) : adapter.url(req);
if (url != null) customizer.tag(TraceKeys.HTTP_URL, url);
super.request(adapter, req, customizer);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public ResultSetInternalMethods preProcess(String sql, Statement interceptedStat
}
int spaceIndex = sql.indexOf(' '); // Allow span names of single-word statements like COMMIT
span.kind(Span.Kind.CLIENT).name(spaceIndex == -1 ? sql : sql.substring(0, spaceIndex));
span.tag("sql.query", sql);
doPreProcessTag(connection, sql, span);
parseServerAddress(connection, span);
span.start();
}
Expand All @@ -46,6 +46,10 @@ public ResultSetInternalMethods preProcess(String sql, Statement interceptedStat
return null;
}

protected void doPreProcessTag(Connection connection, String sql, Span span) {
span.tag("sql.query", sql);
}

/**
* There's no attribute namespace shared across request and response. Hence, we need to save off
* a reference to the span in scope, so that we can close it in the response.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package brave.mysql.aws;

import brave.Span;
import brave.mysql.TracingStatementInterceptor;
import com.mysql.jdbc.Connection;
import java.sql.DatabaseMetaData;
import java.sql.SQLException;
import java.util.logging.Level;
import java.util.logging.Logger;

public class XRayTracingStatementInterceptor extends TracingStatementInterceptor {

private final static Logger LOGGER = Logger.getLogger(XRayTracingStatementInterceptor.class.getName());

private final boolean useQueryForUrl;

public XRayTracingStatementInterceptor() {
this(false);
}

public XRayTracingStatementInterceptor(boolean useQueryForUrl) {
this.useQueryForUrl = useQueryForUrl;
}

@Override
protected void doPreProcessTag(Connection connection, String sql, Span span) {
Copy link
Member

Choose a reason for hiding this comment

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

you've probably seen the http client parser stuff in brave-instrumentation-http. This hints at a similar one brave-instrumentation-sql. If you are up to it, probably a good thing to make a module as most of these properties will be re-used across p6spy and the two mysql variants. This will reduce the amount of copy/pasting and also decouple dependencies when you go to make brave-parser-xray

super.doPreProcessTag(connection, sql, span);
DatabaseMetaData m;
try {
m = connection.getMetaData();
span.tag("sql.url", useQueryForUrl ? sql : m.getURL());
span.tag("sql.database_type", m.getDatabaseProductName());
span.tag("sql.database_version", m.getDatabaseProductVersion());
span.tag("sql.driver_version", m.getDriverVersion());
span.tag("sql.user", m.getUserName());
span.tag("sql.query", sql);
} catch (SQLException e) {
LOGGER.log(Level.WARNING, "DatabaseMetaData could not be get.");
}

}
}