-
Notifications
You must be signed in to change notification settings - Fork 150
feat(trace-exporter): setup boilerplate for data-pipeline integration #6314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d26940d
a8c7904
6d6145e
b7cf051
3de4ab7
b09362d
b493a05
c8bbdf5
ede3d4a
635949c
b38f13b
db88db4
f8f3a32
7ad96e9
fde7edb
2da2b69
fe2d531
22c06df
d83d22d
74c9f35
1959aff
e2cad01
c18b698
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,6 +186,11 @@ await Task.WhenAny(_flushTask, Task.Delay(TimeSpan.FromSeconds(20))) | |
| await _statsAggregator.DisposeAsync().ConfigureAwait(false); | ||
| } | ||
|
|
||
| if (_api is IDisposable disposableApi) | ||
| { | ||
| disposableApi.Dispose(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory, yes, we shouldn't be doing it this way, but the current code architecture doesn't allow for a better solution. Both @andrewlock and I came to the conclusion that this is the best place to dispose of the native component. This is especially important in cases where there can be multiple AgentWriter instances (one winding down while another is starting). Historically, the .NET SDK was designed to re-initialize the pipeline when major configuration changes occur that affect the AgentWriter. Hence, coupling the TraceExporter with AgentWriter is ideal for us, as it allows different configurations for different pipelines while ensuring proper disposal of resources. |
||
| } | ||
|
|
||
| if (!success) | ||
| { | ||
| Log.Warning("Could not flush all traces before process exit"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |||||
| using System.Linq; | ||||||
| using System.Text; | ||||||
| using System.Text.RegularExpressions; | ||||||
| using Datadog.Trace.Agent; | ||||||
| using Datadog.Trace.Ci; | ||||||
| using Datadog.Trace.Ci.CiEnvironment; | ||||||
| using Datadog.Trace.ClrProfiler; | ||||||
|
|
@@ -413,6 +414,38 @@ _ when x.ToBoolean() is { } boolean => boolean, | |||||
| .AsBoolResult() | ||||||
| .OverrideWith(in otelRuntimeMetricsEnabled, ErrorLog, defaultValue: false); | ||||||
|
|
||||||
| DataPipelineEnabled = config | ||||||
| .WithKeys(ConfigurationKeys.TraceDataPipelineEnabled) | ||||||
| .AsBool(defaultValue: true); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a reminder to set this false before the final merge 🙂
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So are we or are we not changing the default to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We removed it from that PR, so we should reinstated it here, right @ganeshnj?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was talking offline, we can merge with enabled by default I will add a different PR to disable and title it MERGE BEFORE RELEASE this will allow us to exercise these changes for all PR and merges. But we have to 100% sure that we don't accidentally do a release without merging the default disable PR. |
||||||
|
|
||||||
| if (DataPipelineEnabled) | ||||||
| { | ||||||
| // Due to missing quantization and obfuscation in native side, we can't enable the native trace exporter | ||||||
| // as it may lead to different stats results than the managed one. | ||||||
| if (StatsComputationEnabled) | ||||||
| { | ||||||
| DataPipelineEnabled = false; | ||||||
| Log.Warning( | ||||||
| "{ConfigurationKey} is enabled, but {StatsComputationEnabled} is enabled. Disabling {TraceDataPipelineEnabled}.", | ||||||
| ConfigurationKeys.TraceDataPipelineEnabled, | ||||||
| ConfigurationKeys.StatsComputationEnabled, | ||||||
| ConfigurationKeys.TraceDataPipelineEnabled); | ||||||
| _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated); | ||||||
| } | ||||||
|
|
||||||
| // Windows supports UnixDomainSocket https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/ | ||||||
| // but tokio hasn't added support for it yet https://github.com/tokio-rs/tokio/issues/2201 | ||||||
| if (Exporter.TracesTransport == TracesTransportType.UnixDomainSocket && FrameworkDescription.Instance.IsWindows()) | ||||||
| { | ||||||
| DataPipelineEnabled = false; | ||||||
| Log.Warning( | ||||||
| "{ConfigurationKey} is enabled, but TracesTransport is set to UnixDomainSocket which is not supported on Windows. Disabling {TraceDataPipelineEnabled}.", | ||||||
| ConfigurationKeys.TraceDataPipelineEnabled, | ||||||
| ConfigurationKeys.TraceDataPipelineEnabled); | ||||||
| _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // We should also be writing telemetry for OTEL_LOGS_EXPORTER similar to OTEL_METRICS_EXPORTER, but we don't have a corresponding Datadog config | ||||||
| // When we do, we can insert that here | ||||||
|
|
||||||
|
|
@@ -1061,6 +1094,12 @@ public bool DiagnosticSourceEnabled | |||||
| /// </summary> | ||||||
| internal bool RuntimeMetricsEnabled => DynamicSettings.RuntimeMetricsEnabled ?? _runtimeMetricsEnabled; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets a value indicating whether libdatadog data pipeline | ||||||
| /// is enabled. | ||||||
| /// </summary> | ||||||
| internal bool DataPipelineEnabled { get; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets the comma separated list of url patterns to skip tracing. | ||||||
| /// </summary> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| // <copyright file="ByteSlice.cs" company="Datadog"> | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. | ||
| // </copyright> | ||
|
|
||
| #nullable enable | ||
|
|
||
| using System; | ||
| using System.Runtime.InteropServices; | ||
|
|
||
| namespace Datadog.Trace.LibDatadog; | ||
|
|
||
| /// <summary> | ||
| /// Represents a slice of a byte array in memory. | ||
| /// </summary> | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| internal struct ByteSlice | ||
| { | ||
| /// <summary> | ||
| /// Pointer to the start of the slice. | ||
| /// </summary> | ||
| internal nint Ptr; | ||
|
|
||
| /// <summary> | ||
| /// Length of the slice. | ||
| /// </summary> | ||
| internal nuint Len; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| // <copyright file="CharSlice.cs" company="Datadog"> | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. | ||
| // </copyright> | ||
|
|
||
| #nullable enable | ||
|
|
||
| using System; | ||
| using System.Runtime.InteropServices; | ||
|
|
||
| namespace Datadog.Trace.LibDatadog; | ||
|
|
||
| /// <summary> | ||
| /// Represents a slice of a UTF-8 encoded string in memory. | ||
| /// </summary> | ||
| [StructLayout(LayoutKind.Sequential)] | ||
| internal struct CharSlice : IDisposable | ||
| { | ||
| /// <summary> | ||
| /// Pointer to the start of the slice. | ||
| /// </summary> | ||
| internal nint Ptr; | ||
|
|
||
| /// <summary> | ||
| /// Length of the slice. | ||
| /// </summary> | ||
| internal nuint Len; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="CharSlice"/> struct. | ||
| /// This can be further optimized if we can avoid copying the string to unmanaged memory. | ||
| /// </summary> | ||
| /// <param name="str">The string to copy into memory.</param> | ||
| internal CharSlice(string? str) | ||
| { | ||
| if (str == null) | ||
| { | ||
| Ptr = IntPtr.Zero; | ||
| Len = UIntPtr.Zero; | ||
andrewlock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| else | ||
| { | ||
| // copy over str to unmanaged memory | ||
| var bytes = System.Text.Encoding.UTF8.GetBytes(str); | ||
| Ptr = Marshal.AllocHGlobal(bytes.Length); | ||
| Marshal.Copy(bytes, 0, Ptr, bytes.Length); | ||
lucaspimentel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Len = (nuint)bytes.Length; | ||
| } | ||
| } | ||
|
|
||
| public void Dispose() | ||
lucaspimentel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| Marshal.FreeHGlobal(Ptr); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.