Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions MySQL.Data/src/MySQLActivitySource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ internal static void SetException(Activity activity, Exception ex)
}


internal static Activity CommandStart(MySqlCommand command)
internal static Activity CommandStart(MySqlCommand command, string sql)
{
if (!Active) return null;

Expand All @@ -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);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

activity?.SetTag("db.statement", sql);
activity?.SetTag("thread.id", Thread.CurrentThread.ManagedThreadId);
activity?.SetTag("thread.name", Thread.CurrentThread.Name);
if (command.CommandType == CommandType.TableDirect)
Expand Down
2 changes: 1 addition & 1 deletion MySQL.Data/src/MySqlCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif
try
{
Expand Down
12 changes: 12 additions & 0 deletions MySQL.Data/tests/MySql.Data.Tests/CmdTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,10 @@ public void DontAllowBatching()
using (MySqlConnection c = new MySqlConnection(connStr.GetConnectionString(true)))
{
c.Open();

using var _ = AddActivityListener(
(activity) => Assert.AreEqual("SELECT 1", activity.GetTagItem("db.statement"))
);
MySqlCommand cmd = new MySqlCommand("SELECT 1", c);
cmd.ExecuteScalar();
}
Expand All @@ -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) =>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test would not have passed without code change

{
Assert.AreEqual("Test,Test1", activity.GetTagItem("db.sql.table"));
Assert.AreEqual("SELECT * FROM Test,Test1", activity.GetTagItem("db.statement"));
}
);

MySqlCommand cmd = new MySqlCommand("Test,Test1", Connection);
cmd.CommandType = CommandType.TableDirect;
using (MySqlDataReader reader = cmd.ExecuteReader())
Expand Down Expand Up @@ -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")));
Copy link
Author

@Zirak Zirak Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test would not have passed without code change

cmd.CommandText = "SELECT * FROM Test";
int id = 1;

Expand Down
31 changes: 31 additions & 0 deletions MySQL.Data/tests/MySql.Data.Tests/TestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// here, the logic is incomplete, etc. Was good for a few hacky tests
public static ActivityListener AddActivityListener(Action<Activity> activityCheck)
{
var listener = new ActivityListener
{
ShouldListenTo = source =>
{
return source.Name == "connector-net";
},
ActivityStopped = activity =>
{
Assert.AreEqual("mysql", activity.GetTagItem("db.system"));

if (activityCheck == null)
{
Assert.Fail("Activity raised without a checking function");
}
else
{
activityCheck(activity);
}
},
Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData,
};

ActivitySource.AddActivityListener(listener);

return listener;
}
#endregion
}
}