Add config to control tracing sql parameters in MySQL agent#2846
Add config to control tracing sql parameters in MySQL agent#2846wu-sheng merged 37 commits intoapache:masterfrom kezhenxu94:gh/2587
Conversation
wu-sheng
added
agent
You could simply ignore them. I don't think we need to do this with such high payload. For
|
|
Also, active this in MySQL integration test case, and modify the test case, please. |
The reason why I'd like to do this is to make it a placeholder, as you said:
keeping a
quoting the parameter with |
|
Should we limit the length of string parameters? The performance of Agent will be reduced if the values of SQL parameter are too large |
You mean truncating the final parameter string? If users active this config but not get the complete parameters, it would be useless sometimes (the useful parameters may be truncated). I'd rather suggest the users not to active this config when the parameters are large/long, or just active during debugging. It's disabled by default, so the users must pay for the performance themselves if we clearly documented this. Just my own opinion, further discussion are very welcomed. @ascrutae |
This could be argued too :) But for performance concern, I think @kezhenxu94 you could set a max parameter length config too. |
Normally, I would say JSON Array should a good idea, but in agent, I think we don't have JSON lib today. |
There seems to be GSON already |
If so, using it is a better idea. What do you think? |
I'm with it |
Hi @ascrutae , as suggested, I've truncate the sql parameters if it's long. now it works like this:
As @wu-sheng suggested, I've made another config |
|
OK. I am OK with this PR at high level. @IanCao @ascrutae Please do the code receivews. @kezhenxu94 Let's make the test cases work. |
Turns out that we cannot use the Gson library because of the same reason as #2871 |
wu-sheng
left a comment
wu-sheng
left a comment
There was a problem hiding this comment.
LGTM. Could you run the test case locally or ask @IanCao to run the cases on our server.
I think it would be safer to recheck the test cases. @kezhenxu94 If you want to run server, you need to tell @IanCao which cases are needed to run. All MySQL cases, I guess?
Yes all MySQL cases. @IanCao Could you please run the tests with the original test code and agent code of this PR? I'll run the modified version locally and post the result here |
Our tests will use these PR codes, not original codes. @kezhenxu94 |
@wu-sheng What I meant is to ask @IanCao to use the original test codes to test these PR codes (with the config key off, which is by default), and in order to test the codes with the config key on, I have to modify some test codes, and I'll use the modified test codes to test these PR code, make sense? |
|
Here is the test report and validate logs |
|
@kezhenxu94 Look like most passed. One fails, I assume it is just accidental by current test tool, cc @ascrutae . |
|
@kezhenxu94 I am waiting for your test results of |
Tests are still running locally, poor local machine 😢 once done, I'll post here |
Got it. @ascrutae Let's make this work in next month, then we could have a much more friendly PR procedure. |
Test Report - 2019-06-19 11:52
Cases List
Mysql
And FYI, my test codes are here |
5 participants
Please answer these questions before submitting pull request
Why submit this pull request?
Bug fix
New feature provided
Improve performance
Related issues
resolves Support database sql statement parameter value(optional) #2587
New feature or improvement
Add a config key to control whether to collect the SQL parameters.
Here is one point need to discuss:
there're methods like
java.sql.PreparedStatement#setBinaryStream(int, java.io.InputStream, long),java.sql.PreparedStatement#setBlob(int, java.io.InputStream, long)and similar methods inPreparedStatement, should we collect them as well? TheInputStreammay be large and hard to display in tags, I would like to only collect their length and display something likeInputStream[length]or anyone has better idea?cc @vision-ken
here is the span detail:
