Add support for including sql query in sql subsegment for MySQL #564
Conversation
…all to createSqlData to send those values from the arguments made to the sql call
|
Hi, updated to include the fixes to set SqlData values from arguments as opposed to the results from query/execute (which may be a promise and not have values) and to fix a bug when promises are returned by query/execute and the promise rejects. |
Codecov Report
@@ Coverage Diff @@
## master #564 +/- ##
=======================================
Coverage 83.38% 83.38%
=======================================
Files 37 37
Lines 1794 1794
=======================================
Hits 1496 1496
Misses 298 298 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Hi - friendly "bump" - anything I can do to help this progress? |
| function createSqlData(config, command) { | ||
| var commandType = command.values ? PREPARED : null; | ||
|
|
||
| function createSqlData(config, values, sql) { |
There was a problem hiding this comment.
Hi, thanks for this contribution!
You mentioned this issue + the fix for it in the bug report, maybe keeping createSqlData(config, args) with 2 parameters might be better to remain consistent with the same function in postgres?
There was a problem hiding this comment.
Well... other than the number of arguments, the calls to createSqlData are not very consistent in the current state, primarily because the value getting passed in to createSqlData in mysql_p can either be a command or a promise to return results (which is the whole reason this PR exists in the first place).
As far as the number of arguments to createSqlData in mysql_p.js, I could pass in the args which consistently contains both the SQL and parameters, which would keep it at two arguments; albeit the second argument by necessity is different than what gets passed in to createSqlData in postgres_p.js. In untyped languages I always found it easier to explicitly accept discrete values as opposed to a composited object, but it's a nit. If two arguments is important, I can make that work and resubmit so I can log SQL with MySQL in XRay.
There was a problem hiding this comment.
Yea they definitely do different things, this was just a nit pick to target as much consistency as possible. There isn't a strong preference for either option on my end, so I'll leave that decision up to you. I think a small comment explaining how/why this function is different can also be very helpful for anyone looking back on this implementation
There was a problem hiding this comment.
Ok, added function notes per your request
| @@ -141,7 +141,7 @@ describe('captureMySQL', function() { | |||
| var stubDataInit = sandbox.stub(SqlData.prototype, 'init'); | |||
| var config = conn.config; | |||
|
|
|||
| query.call(connectionObj, 'sql here'); | |||
| query.call(connectionObj, 'sql here', [1]); | |||
There was a problem hiding this comment.
Just trying to get a better understanding of this, do you mind clarifying the purpose of passing in [1] here?
There was a problem hiding this comment.
That "[1]" mocks query parameters, the existence of which is used in mysql_p to determine whether a statement is prepared or not (this is current state, I did not change this). FWIW, this is different than how postgres_p.createSqlData determines a prepared statement, which is to look for a name of the query object, which allows the statement to be cached on the server. My understanding is that MySQL doesn't support similar named prepared statement functionality, thus the check for values, and thus the unit test's "[1]"
|
Ok, I give up... I don't know what Lerna is, and I do not have the time to learn. Literally the same code was working two weeks ago with only an additional comment added. |
@jasonterando apologies for the confusion. This is a CI issue that is not related to your changes, I'll be looking into it and will merge this PR once it's fixed |
Issue: 563
Description of changes: Add support for adding sql query to sql subsegment when AWS_XRAY_COLLECT_SQL_QUERIES is set.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.