[Instrumentation.Kusto] Add instrumentor for Azure Kusto#3591
[Instrumentation.Kusto] Add instrumentor for Azure Kusto#3591MattKotsenas wants to merge 26 commits intoopen-telemetry:mainfrom
Conversation
0aef4ac to
77819be
Compare
src/OpenTelemetry.Instrumentation.Kusto/Implementation/KustoTraceRecordListener.cs
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.Kusto/Implementation/KustoTraceRecordListener.cs
Show resolved
Hide resolved
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Please do not close due to staleness. There won't be much movement on this over the holidays, but I'll return to this in the new year. Thanks! |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Code Review – Key FindingsSummary: Issues
|
a651651 to
9a5e236
Compare
|
/cc @rajkumar-rangaraj, ready to restart the review process |
The semantic analysis is required when summarizing in order to flag table-like elements. Per the benchmarks, I'm seeing single digit microseconds for summarization + sanitization. The slightly bigger issue we might run into is the allocations that the KustoCode parser allocates. We can avoid both with a query cache, which I didn't add pre-emptively because a cache without an expiration policy is a memory leak, but am happy to revisit.
I'm happy to do this if we feel it's really necessary, however from a sanitization perspective I'm not sure the complexity is worth the cost. |
|
The PR does not update Maintaining code owners is an important requirement for this repo, and each component should have at least two owners. |
|
The PR puts options in
The predominant repo pattern (SqlClient, Http, etc.) puts options in the instrumentation's own
Consider moving to |
|
@Kielek / @martincostello What are your thoughts on it? |
src/OpenTelemetry.Instrumentation.Kusto/OpenTelemetry.Instrumentation.Kusto.csproj
Show resolved
Hide resolved
Who should I put here? I'm happy to put myself as one, but hadn't originally because I believe I need to be sponsored to do so (would be honored :)). |
I'm happy to introduce it. I use it a lot myself, and we also use it in open-telemetry/opentelemetry-dotnet (added in open-telemetry/opentelemetry-dotnet#6567). |
There was a problem hiding this comment.
There's some other settings we should add for Verify, such as changes to .editorconfig settings. See here for some of the missing parts.
| <PackageVersion Include="Microsoft.Data.SqlClient" Version="7.0.0" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Sqlite" Version="8.0.25" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.SqlServer" Version="8.0.25" /> | ||
|
|
| ## Unreleased | ||
|
|
||
| For more details, please refer to the [README](README.md). | ||
| * Initial implementation. |
There was a problem hiding this comment.
| * Initial implementation. | |
| * Initial implementation. | |
| ([#3591](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/3591)) |
| /// <summary> | ||
| /// Helper class to hold common properties used by Kusto instrumentation. | ||
| /// </summary> | ||
| internal static class KustoActivitySourceHelper |
There was a problem hiding this comment.
Given this contains activities and meters, I would give it a different name.
| { | ||
| private static readonly Lazy<ITraceListener> Listener = new(() => | ||
| { | ||
| Environment.SetEnvironmentVariable("KUSTO_DATA_TRACE_REQUEST_BODY", "1"); |
There was a problem hiding this comment.
I'm not sure we should be changing process env vars internally. If this is some internal implementation detail like how the Azure SDKs need (needed?) an opt-in to emit OTel metrics, I would instead document that the user is required to set that themselves.
It's also messy for tests as it changes the test environment.
| { | ||
| services.Configure<KustoTraceInstrumentationOptions>(options => | ||
| { | ||
| options.Enrich = (activity, record) => { enrichCalled = true; }; |
There was a problem hiding this comment.
| options.Enrich = (activity, record) => { enrichCalled = true; }; | |
| options.Enrich = (_, _) => enrichCalled = true; |
| /// <summary> | ||
| /// Class to hold the singleton instances used for Kusto instrumentation. | ||
| /// </summary> | ||
| internal static class KustoInstrumentation |
There was a problem hiding this comment.
Is there a way to design this without needing statics? Then the tests can be simpler too.
| => new KustoBuilder(KustoImage) | ||
| .Build(); |
There was a problem hiding this comment.
| => new KustoBuilder(KustoImage) | |
| .Build(); | |
| => new KustoBuilder(KustoImage).Build(); |
| public DependencyInjectionConfigTests() | ||
| { | ||
| KustoInstrumentation.TraceOptions = new KustoTraceInstrumentationOptions(); | ||
| KustoInstrumentation.MeterOptions = new KustoMeterInstrumentationOptions(); | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| KustoInstrumentation.TraceOptions = new KustoTraceInstrumentationOptions(); | ||
| KustoInstrumentation.MeterOptions = new KustoMeterInstrumentationOptions(); | ||
| } |
There was a problem hiding this comment.
The need to keep mutating statics feels like a smell to me.
| using var tracerProvider = Sdk.CreateTracerProviderBuilder() | ||
| .AddInMemoryExporter(activities) | ||
| .AddKustoInstrumentation(options => | ||
| { | ||
| options.RecordQueryText = processQuery; | ||
| options.RecordQuerySummary = processQuery; | ||
| }) | ||
| .Build(); | ||
|
|
||
| using var meterProvider = Sdk.CreateMeterProviderBuilder() | ||
| .AddInMemoryExporter(metrics) | ||
| .AddKustoInstrumentation(options => | ||
| { | ||
| options.RecordQueryText = processQuery; | ||
| options.RecordQuerySummary = processQuery; | ||
| }) | ||
| .Build(); | ||
|
|
||
| var kcsb = this.fixture.ConnectionStringBuilder; | ||
| using var queryProvider = KustoClientFactory.CreateCslQueryProvider(kcsb); | ||
|
|
||
| var crp = new ClientRequestProperties() | ||
| { | ||
| // Ensure a stable client ID for snapshots | ||
| ClientRequestId = Convert.ToBase64String(Encoding.UTF8.GetBytes(query)), | ||
| }; | ||
|
|
||
| using var reader = queryProvider.ExecuteQuery("NetDefaultDB", query, crp); | ||
| reader.Consume(); | ||
|
|
||
| tracerProvider.ForceFlush(); | ||
| meterProvider.ForceFlush(); |
There was a problem hiding this comment.
I would wrap all these in an explicit using - we've had some test flakiness in the past where activities/metrics get registered after the flush, and then the asserts fail as something's missing. The extra dispose before asserting helps avoid that.
| public static readonly string PackageVersion = Assembly.GetPackageVersion(); | ||
|
|
||
| public static readonly string ActivitySourceName = AssemblyName.Name!; | ||
| public static readonly ActivitySource ActivitySource = new(ActivitySourceName, PackageVersion); |
There was a problem hiding this comment.
We should also include a schema URL like we do here:
Add an option to disable query summarization
63134a9 to
3a97216
Compare
| /// </summary> | ||
| internal static class KustoActivitySourceHelper | ||
| { | ||
| public const string DbSystem = "azure.kusto"; |
There was a problem hiding this comment.
ConiderDbSystemNameValue / KustoDbSystemNameValue / AzureKustoDbSystemNameValue
Fixes #3575
Changes
Adds an initial implementation of traces and metrics for any client built on top of the
Microsoft.Azure.Kusto.Cloud.Platformpackage (namely Microsoft.Azure.Kusto.Data and Microsoft.Azure.Kusto.Ingest).The library generally follows the semantic conventions for dbs for both traces and metrics, importantly, it does both query summarization and sanitization. It does not support adding query parameters as tags, as those aren't available in Kusto's internal custom tracing framework. Test coverage includes unit tests, benchmarks both for end-to-end instrumentation and query processing specific, and integration tests using the existing pattern of Testcontainers.
The package is not strong-name signed because it uses
Microsoft.Azure.Kusto.Languagefor query processing, which also is not strong-name signed.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes