WIP Ensure sql statements end up in ActivitySource #63
Conversation
There are quite a few ways for an sql statement to wound up being executed, including but not only: 1. Passing it into the `MySQLCommand` constructor 2. Assigning a `MySQLCommand` instance's `CommandText` 3. Using `CommandType.TableDirect` with a table name 4. Using a stored procedure 5. ...with parameters 6. Opening a connection And probably more. The existing bridge into the `ActivitySource` answers some of them, and this commit hopes to bridge some of the gap. Namely: - Scenario 2 (`CommandText` assignment) - Scenario 3 (`TableDirect`) shows the actually executing SQL This commit is currently to be considered in a draft state; it is not ready to be merged. Inline review comments will be added for clarifications and questions.
| @@ -81,7 +81,7 @@ internal static Activity CommandStart(MySqlCommand command) | |||
| activity?.SetTag("db.system", "mysql"); | |||
| activity?.SetTag("db.name", command.Connection.Database); | |||
| activity?.SetTag("db.user", command.Connection.Settings.UserID); | |||
| activity?.SetTag("db.statement", command.OriginalCommandText); | |||
There was a problem hiding this comment.
OriginalCommandText was a bit of a mystery to me - it may have made sense when the code looked different, but I don't understand when the sql query at time of call isn't good. I analysed the code paths in the PR description and they all seemed very understandable.
Is there an edge case I'm missing?
| @@ -760,7 +760,7 @@ internal async Task<MySqlDataReader> ExecuteReaderAsync(CommandBehavior behavior | |||
|
|
|||
| // Tell whoever is listening that we have started out command | |||
| #if NET5_0_OR_GREATER | |||
| CurrentActivity = MySQLActivitySource.CommandStart(this); | |||
| CurrentActivity = MySQLActivitySource.CommandStart(this, sql); | |||
There was a problem hiding this comment.
While I wanted to avoid passing in the sql variable and use the MySqlCommand, it seemed most appropriate - db.statement reflects the statement to be executed by the database. The sql variable is most closely that.
| @@ -333,6 +333,37 @@ public string GetMySqlServerIp(bool isIpV6 = false) | |||
|
|
|||
| return isIpV6 ? ipv6 : ipv4; | |||
| } | |||
|
|
|||
| // XXX This shouldn't be the final form - it probably shouldn't be placed | |||
There was a problem hiding this comment.
This isn't very good; I'm fairly new to dotnet unit testing and NUnit, so I tried to be a little generic but not too much. In general I wasn't sure whether to make specific tests for MySQLActivitySource, since it seemed like it would be duplicating all the great tests that currently exist.
Something I was doubly unsure of: This isn't relevant for runtimes which don't have MySQLActivitySource.
| @@ -949,6 +960,7 @@ public void LastInsertedIdInMultipleStatements() | |||
| cmd.ExecuteNonQuery(); | |||
| Assert.AreEqual(2, cmd.LastInsertedId); | |||
|
|
|||
| using var _ = AddActivityListener((activity) => Assert.AreEqual("SELECT * FROM Test", activity.GetTagItem("db.statement"))); | |||
There was a problem hiding this comment.
This test would not have passed without code change
| @@ -490,6 +494,13 @@ public void TableCommandType() | |||
| ExecuteSQL("CREATE TABLE Test1 (id INT, name VARCHAR(20))"); | |||
| ExecuteSQL("INSERT INTO Test1 VALUES (2, 'B')"); | |||
|
|
|||
| using var _ = AddActivityListener((activity) => | |||
There was a problem hiding this comment.
This test would not have passed without code change
|
Hi, thank you for submitting this pull request. In order to consider your code we need you to sign the Oracle Contribution Agreement (OCA). Please review the details and follow the instructions at https://oca.opensource.oracle.com/ |
|
Hi, thank you for your contribution. Please confirm this code is submitted under the terms of the OCA (Oracle's Contribution Agreement) you have previously signed by cutting and pasting the following text as a comment: |
|
I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it. |
|
Hi, thank you for your contribution. Your code has been assigned to an internal queue. Please follow |
There are quite a few ways for an sql statement to wound up being executed, including but not only:
MySQLCommandconstructorMySQLCommandinstance'sCommandTextCommandType.TableDirectwith a table nameAnd probably more. The existing bridge into the
ActivitySourceanswers some of them, and this commit hopes to bridge some of the gap. Namely:CommandTextassignment)TableDirect) shows the actually executing SQLThis commit is currently to be considered in a draft state; it is not ready to be merged. Inline review comments will be added for clarifications and questions.