-
Notifications
You must be signed in to change notification settings - Fork 150
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
Obfuscate sql for external #559
Draft
XiXiaPdx
wants to merge
12
commits into
main
Choose a base branch
from
obfuscate-sql-for-external
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
ac0187b
obfuscate sql that is above slow query threshold
XiXiaPdx ae2c7cb
refactor names
XiXiaPdx 2ee648f
clean up warnings
XiXiaPdx 39dd747
name change
XiXiaPdx 9dfa371
remove setting of raw string on agent attribute
XiXiaPdx f456603
better name and documentation
XiXiaPdx 12549ec
WIP address this todo
XiXiaPdx cb010af
remove unnecessary queryConverter
XiXiaPdx e91a277
refactor query parameter use
XiXiaPdx 96ccc30
remove todo and add comments
XiXiaPdx 8cc788b
Setting correct attribute name for the sql in tracer attrs
meiao afc1ab4
Switch to Gradle Github Action
twcrone File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -201,7 +201,6 @@ protected void doFinish(Throwable throwable) { | |
String appName = getTransaction().getApplicationName(); | ||
SqlQueryConverter converter = new SqlQueryConverter(appName, getDatabaseVendor()); | ||
String obfuscatedQueryString = converter.toObfuscatedQueryString(sql.toString()); | ||
|
||
// Store the obfuscated query string | ||
getTransaction().getIntrinsicAttributes().put(SQL_PARAMETER_NAME, obfuscatedQueryString); | ||
} | ||
|
@@ -264,13 +263,17 @@ public boolean isMetricProducer() { | |
@Override | ||
protected void recordMetrics(TransactionStats transactionStats) { | ||
if (isMetricProducer() && getTransaction() != null) { | ||
String rawSql = null; | ||
String slowQuerySql = null; | ||
Object sqlObject = getSql(); | ||
String appName = getTransaction().getApplicationName(); | ||
|
||
if (sqlObject != null) { | ||
rawSql = new PreparedStatementSql(sql, params).toString(); | ||
slowQuerySql = new PreparedStatementSql(sql, params).toString(); | ||
if (queryExceedsSlowQueryThreshold(appName)) { | ||
slowQuerySql = getNoRawOrObfuscatedSql(slowQuerySql, appName); | ||
} | ||
} | ||
|
||
String appName = getTransaction().getApplicationName(); | ||
String hostToReport = DatastoreMetrics.replaceLocalhost(getHost()); | ||
|
||
if (getIdentifier() != null) { | ||
|
@@ -280,7 +283,7 @@ protected void recordMetrics(TransactionStats transactionStats) { | |
.operation(parsedDatabaseStatement.getOperation()) | ||
.instance(hostToReport, getIdentifier()) | ||
.databaseName(getDatabaseName()) | ||
.slowQuery(rawSql, new SqlQueryConverter(appName, getDatabaseVendor())) | ||
.slowQuery(slowQuerySql, new SqlQueryConverter(appName, getDatabaseVendor())) | ||
.build()); | ||
} else { | ||
String portToReport = DatastoreMetrics.replacePort(getPort()); | ||
|
@@ -290,7 +293,7 @@ protected void recordMetrics(TransactionStats transactionStats) { | |
.operation(parsedDatabaseStatement.getOperation()) | ||
.instance(hostToReport, portToReport) | ||
.databaseName(getDatabaseName()) | ||
.slowQuery(rawSql, new SqlQueryConverter(appName, getDatabaseVendor())) | ||
.slowQuery(slowQuerySql, new SqlQueryConverter(appName, getDatabaseVendor())) | ||
.build()); | ||
} | ||
|
||
|
@@ -301,6 +304,20 @@ protected void recordMetrics(TransactionStats transactionStats) { | |
super.recordMetrics(transactionStats); | ||
} | ||
|
||
/** | ||
* This method is named this way because {@link SqlQueryConverter#toObfuscatedQueryString(String)} has | ||
* unexpected conditional logic. | ||
*/ | ||
private String getNoRawOrObfuscatedSql(String rawSql, String appName) { | ||
SqlQueryConverter converter = new SqlQueryConverter(appName, getDatabaseVendor()); | ||
return converter.toObfuscatedQueryString(rawSql); | ||
} | ||
|
||
private boolean queryExceedsSlowQueryThreshold(String appName) { | ||
double threshold = ServiceFactory.getConfigService().getAgentConfig(appName).getTransactionTracerConfig().getExplainThresholdInMillis(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing we know that none of these calls can return a null and cause an NPE |
||
return getDurationInMilliseconds() > threshold; | ||
} | ||
|
||
/** | ||
* Returns the sql for the statement instrumented by this driver. This method is overridden by prepared statement | ||
* tracers which return a sql object that can delay the rendering of the full sql statement with parameters until it | ||
|
@@ -499,7 +516,7 @@ public String toString() { | |
* @param parameters the parameter map | ||
* @return the parameterized SQL | ||
*/ | ||
public static String parameterizeSql(String sql, Object[] parameters) throws Exception { | ||
public static String parameterizeSql(String sql, Object[] parameters) { | ||
if (sql == null || parameters == null || parameters.length == 0) { | ||
return sql; | ||
} | ||
|
@@ -512,11 +529,11 @@ public static String parameterizeSql(String sql, Object[] parameters) throws Exc | |
} else { | ||
Object val = i < parameters.length ? parameters[i] : null; | ||
if (val instanceof Number) { | ||
sb.append(piece).append(val.toString()); | ||
sb.append(piece).append(val); | ||
} else if (val == null) { | ||
sb.append(piece).append("?"); | ||
} else { | ||
sb.append(piece).append("'").append(val.toString()).append("'"); | ||
sb.append(piece).append("'").append(val).append("'"); | ||
} | ||
} | ||
} | ||
|
@@ -537,6 +554,12 @@ public String toRawQueryString(String rawQuery) { | |
return rawQuery; | ||
} | ||
|
||
/** | ||
* For this implementation, the getSqlfuscator has conditional | ||
* logic to return an obfuscator that respects the agent configuration setting `record_sql` | ||
* | ||
* See {@link com.newrelic.agent.database.DatabaseService#createSqlObfuscator(TransactionTracerConfig) } | ||
*/ | ||
@Override | ||
public String toObfuscatedQueryString(String rawQuery) { | ||
SqlObfuscator sqlObfuscator = ServiceFactory.getDatabaseService().getSqlObfuscator(appName); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixed a ClassCastException, but it is an oversimplification that may not work for all query objects from different databases.
Ideally here it should use the queryConverter in the SlowQueryDatastoreParameters instead of assuming the toString call will return a query.