Issue#1435 #1584
Issue#1435 #1584
Conversation
|
Using the I don't understand the purpose of adding |
There was a problem hiding this comment.
Pull Request Overview
This PR adds telemetry support for database schema operations by implementing a new DatabaseTelemetry class. The change introduces OpenTelemetry activity tracking for schema collection operations with support for both legacy and modern semantic conventions.
- Adds telemetry infrastructure with activity source and attribute management
- Implements support for multiple OpenTelemetry semantic convention versions (old, new, both)
- Creates a complete Visual Studio solution file for the project
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/SchemaCollectionGenerator/SchemaCollectionGenerator.cs | Adds DatabaseTelemetry class with OpenTelemetry activity creation for schema operations |
| MySqlConnector.sln | Creates new Visual Studio solution file defining the project structure |
| @@ -271,3 +271,46 @@ internal sealed partial class Regexes | |||
| [GeneratedRegex("[^a-z0-9]", RegexOptions.IgnoreCase | RegexOptions.Compiled, "en-US")] | |||
| public static partial Regex NonAsciiAlphaNumeric(); | |||
| } | |||
| internal static class DatabaseTelemetry | |||
| { | |||
| private static readonly ActivitySource ActivitySource = new("MySqlConnector"); | |||
There was a problem hiding this comment.
The ActivitySource is not being disposed. Consider implementing IDisposable on the class or using a static disposal mechanism to properly clean up the ActivitySource resource.
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| private static bool UseOld => SemConvOptIn?.Equals("old", StringComparison.OrdinalIgnoreCase) ?? false; | ||
| private static bool UseNew => SemConvOptIn?.Equals("new", StringComparison.OrdinalIgnoreCase) ?? false; |
There was a problem hiding this comment.
The default behavior when SemConvOptIn is null is to use 'both' conventions. This should be documented as it's not immediately obvious from the code that null defaults to 'both' rather than a specific convention.
| private static bool UseNew => SemConvOptIn?.Equals("new", StringComparison.OrdinalIgnoreCase) ?? false; | |
| private static bool UseNew => SemConvOptIn?.Equals("new", StringComparison.OrdinalIgnoreCase) ?? false; | |
| // If SemConvOptIn is null or unset, default to using both conventions. |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| activity.SetTag("db.operation", "GetSchema"); | ||
| activity.SetTag("db.sql.table", collectionName); |
There was a problem hiding this comment.
The attribute 'db.sql.table' may not be semantically correct for schema collection operations. Schema operations typically don't target a specific table, and this attribute is meant for table-specific operations. Consider using a more appropriate attribute or documenting why this mapping is used.
| activity.SetTag("db.sql.table", collectionName); | |
| // Removed db.sql.table as it is not semantically correct for schema collection operations. |
Copilot uses AI. Check for mistakes.
No description provided.