[WIP]add postgresql plugin agent sql param show#3618
Conversation
merge from apache
Merge from apache
|
I didn't see rebase :) Still a lot of commits. |
wu-sheng
added
agent
|
You should learn from the existing case(https://github.com/apache/skywalking/tree/master/test/plugin/scenarios/solrj-7.x-scenario), and move the following two into the main repo.
You could ping me on QQ, wechat or mail, as we don't have the document(we are working on that) for the new plugin test development guide. |
|
The old test includes 55 cases, https://github.com/SkyAPMTest/agent-integration-test-report#postgresql. Last time, we passed. This should be easy work to move them in. |
|
@wu-sheng ok,thanks |
|
Any update about the postgre test PR? |
|
WIP |
kezhenxu94
left a comment
kezhenxu94
left a comment
There was a problem hiding this comment.
Haven't looked carefully yet, but just point out some obvious issues, and remember to add the config item into the documentations
| * If set to true, the parameters of the sql (typically {@link java.sql.PreparedStatement}) would be | ||
| * collected. | ||
| */ | ||
| public static boolean TRACE_SQL_PARAMETERS = true; |
There was a problem hiding this comment.
Default value should be false due to possible performance issue
| if (Config.Plugin.PostgreSQL.TRACE_SQL_PARAMETERS) { | ||
| final Set<String> setters = ignorable ? PS_IGNORABLE_SETTERS : PS_SETTERS; | ||
| for (String setter : setters) { | ||
| matcher = matcher.or(named(setter)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Simply collapse this to line 48
if (Config.Plugin.MySQL.TRACE_SQL_PARAMETERS || Config.Plugin.PostgreSQL.TRACE_SQL_PARAMETERS) {
final Set<String> setters = ignorable ? PS_IGNORABLE_SETTERS : PS_SETTERS;
for (String setter : setters) {
matcher = matcher.or(named(setter));
}
}| while ((findPos = sql.indexOf("?", startPos)) > 0) { | ||
| resultSql.append(sql.substring(startPos, findPos)); | ||
| resultSql.append("'"); | ||
| resultSql.append(parameters[index++]); |
There was a problem hiding this comment.
It's unsafe to parse the SQL using ?, are there possibilities when the sql includes ? itself in the value, not a parameter placeholder?
select * from news_table where author=? and title like '%?', I know it's a little tricky, just to remind the risk here
There was a problem hiding this comment.
My point is not to try to parse the SQL, store the parameters in another tag
There was a problem hiding this comment.
Parsing SQL is the next generation after ShardingShpere ( @tristaZero and @terrymanu ) provides the real SQL Parser. We don't do this by ourselves.
There was a problem hiding this comment.
@wu-sheng @kezhenxu94 ACK，i will store the parameters in another tag.
|
Please fix the conflict. And we could continue this now. |
|
This should continue. |
|
ok |
ghost
changed the title
|
Please resolve the conflict. |
|
refer #3695 |
|
Should close this one? |
|
replaced by #3695 |
4 participants
Please answer these questions before submitting pull request
Why submit this pull request?
Bug fix
New feature provided
Improve performance
Related issues
Bug fix
Bug description.
How to fix?
New feature or improvement