Review Exporter Settings by pierotibou · Pull Request #2455 · DataDog/dd-trace-dotnet

pierotibou · 2022-02-17T10:35:03Z

Summary of changes

I've removed DD_APM_RECEIVER_SOCKET as it should not have been added. As it is not documented, we agreed with Lucas that it was ok to break the public API. I've also cleaned the settings to reintroduce TraceAgentUri as the first parameter to take into account and make more explicit the order of precedence. Finally, I've removed the TCP Strategy as it wasn't used and as it doesn't seem we want to go further down that road.

Reason for change

A regression had been introduced as we were not using the Trace Agent Uri as the primary parameter to take into account. We've also seen that we may be able to improve how we configure the transport when Kevin developed the Agent checks in the CLI tool.

Implementation details

Regarding the settings refactoring:

I had to introduce a builder class to do build the immutable settings as it seemed clearer (I could have done everything from the .ctor though).
Removed DD_APM_RECEIVER_SOCKET
Reintroduced TraceAgentUri as the first parameter to take into account
made clearer the order of precedence of the parameters in the code
handled possible wrongly set parameters that would lead to throwing an exception
allow falling back on other configuration when fixing the issue would need to restart the app
moved the settings construction in ImmutableSettings (through a builder, unfortunately this class is public, I couldn't do all the changes I would have liked to do)
Added a bit of logs to explain the issues
Removed the TCP strategy

Testing

Tried to add as much testing as I could by testing both ExporterSettings, ImmutableExporterSettings checking both the modified values and the ones that should have stayed with their default values.

Fixes #2426 (hopefully:))

andrewlock · 2022-02-17T13:00:49Z

Benchmarks Report 🐌

Benchmarks for #2455 compared to master:

All benchmarks have the same speed
All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	861μs	4.18μs	28.6μs	3.17 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	636μs	2.51μs	9.72μs	2.57 KB
#2455	`WriteAndFlushEnrichedTraces`	net472	842μs	4μs	16μs	3.16 KB
#2455	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	667μs	3.5μs	20.7μs	2.57 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	293μs	1.47μs	6.41μs	0.15	19.71 KB
#2455	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#2455	`SendRequest`	netcoreapp3.1	292μs	1.17μs	4.53μs	0.146	19.71 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.63μs	5.56ns	21.5ns	0.0926	0.000798	586 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.53μs	5.09ns	19.7ns	0.00826	0	624 B
#2455	`ExecuteNonQuery`	net472	1.68μs	3.91ns	14.6ns	0.0931	0	586 B
#2455	`ExecuteNonQuery`	netcoreapp3.1	1.51μs	5.61ns	21ns	0.00873	0	624 B

Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`CallElasticsearch`	net472	2.45μs	9.84ns	36.8ns	0.125	0	0	794 B
master	`CallElasticsearch`	netcoreapp3.1	1.76μs	8.36ns	32.4ns	0.0111	0	0	784 B
master	`CallElasticsearchAsync`	net472	2.76μs	13.2ns	56.2ns	0.147	0.00137	0	931 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.78μs	6.55ns	25.4ns	0.0124	0	0	904 B
#2455	`CallElasticsearch`	net472	2.59μs	8.31ns	31.1ns	0.126	0	0	794 B
#2455	`CallElasticsearch`	netcoreapp3.1	1.78μs	3.36ns	12.1ns	0.0108	0	0	784 B
#2455	`CallElasticsearchAsync`	net472	2.78μs	11.4ns	59.2ns	0.147	0.00285	0.00142	931 B
#2455	`CallElasticsearchAsync`	netcoreapp3.1	1.85μs	6.72ns	24.2ns	0.0127	0	0	904 B

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.95μs	13.5ns	88.5ns	0.164	1.05 KB
master	`ExecuteAsync`	netcoreapp3.1	1.96μs	11.2ns	79.9ns	0.0137	1.02 KB
#2455	`ExecuteAsync`	net472	2.93μs	14.6ns	87.3ns	0.164	1.05 KB
#2455	`ExecuteAsync`	netcoreapp3.1	1.96μs	10.9ns	64.5ns	0.0136	1.02 KB

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	6.78μs	22.5ns	84ns	0.364	2.3 KB
master	`SendAsync`	netcoreapp3.1	4.71μs	9.56ns	37ns	0.0309	2.2 KB
#2455	`SendAsync`	net472	6.98μs	38ns	237ns	0.359	2.3 KB
#2455	`SendAsync`	netcoreapp3.1	4.81μs	17.9ns	69.4ns	0.0303	2.2 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.12μs	12.2ns	47.2ns	0.226	1.44 KB
master	`EnrichedLog`	netcoreapp3.1	3.07μs	15ns	60ns	0.021	1.52 KB
#2455	`EnrichedLog`	net472	3.23μs	10.3ns	40ns	0.225	1.44 KB
#2455	`EnrichedLog`	netcoreapp3.1	3.1μs	13.4ns	51.8ns	0.0213	1.52 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	281μs	957ns	3.7μs	0.414	0.138	4.31 KB
master	`EnrichedLog`	netcoreapp3.1	223μs	610ns	2.28μs	0	0	4.19 KB
#2455	`EnrichedLog`	net472	288μs	1.26μs	4.88μs	0.424	0.141	4.31 KB
#2455	`EnrichedLog`	netcoreapp3.1	231μs	690ns	2.67μs	0	0	4.19 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	6.99μs	23.8ns	89.1ns	0.504	3.21 KB
master	`EnrichedLog`	netcoreapp3.1	6.28μs	6.98ns	24.2ns	0.0476	3.58 KB
#2455	`EnrichedLog`	net472	7.21μs	36.5ns	171ns	0.502	3.21 KB
#2455	`EnrichedLog`	netcoreapp3.1	6.38μs	28.8ns	112ns	0.0481	3.58 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`SendReceive`	net472	2.3μs	11.3ns	50.3ns	0.159	0.00111	1 KB
master	`SendReceive`	netcoreapp3.1	2.05μs	5.28ns	19ns	0.0136	0	1 KB
#2455	`SendReceive`	net472	2.27μs	8.64ns	33.5ns	0.158	0	1 KB
#2455	`SendReceive`	netcoreapp3.1	2.04μs	11.2ns	63.4ns	0.0139	0	1 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.82μs	25.3ns	94.8ns	0.288	1.85 KB
master	`EnrichedLog`	netcoreapp3.1	4.93μs	22.2ns	86ns	0.0198	1.47 KB
#2455	`EnrichedLog`	net472	6.03μs	31.6ns	197ns	0.287	1.85 KB
#2455	`EnrichedLog`	netcoreapp3.1	5.24μs	25.7ns	173ns	0.0187	1.47 KB

Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	944ns	5.4ns	39.3ns	0.0705	449 B
master	`StartFinishSpan`	netcoreapp3.1	863ns	2.78ns	10.8ns	0.00602	448 B
master	`StartFinishScope`	net472	1.05μs	4.99ns	21.2ns	0.0829	530 B
master	`StartFinishScope`	netcoreapp3.1	1.02μs	3.62ns	14ns	0.00756	568 B
#2455	`StartFinishSpan`	net472	913ns	4.32ns	17.3ns	0.0705	449 B
#2455	`StartFinishSpan`	netcoreapp3.1	876ns	3.47ns	13.4ns	0.00615	448 B
#2455	`StartFinishScope`	net472	1.1μs	5.16ns	20ns	0.0831	530 B
#2455	`StartFinishScope`	netcoreapp3.1	1.1μs	5.11ns	20.4ns	0.00785	568 B

Add the URL as the first parameter to consider Change the tests as well to order them by order of precedence

There's one use case that still throws, not sure what to do yet

Adds a builder to do it to keep the object immutable Changes the tests Adapted the checkAgentCommand

... by checking default values all the time

Based on the same system as used in DirectLogsSubmissionsSettings

Note that it is a breaking change

it was used only in tests and it doesn't seem we are going further down this road

andrewlock · 2022-04-11T12:54:02Z

+            {
+                if (!Uri.TryCreate(traceAgentUrl, UriKind.RelativeOrAbsolute, out var uri))
+                {
+                    ValidationWarnings.Add($"The provided Uri: ${traceAgentUrl} provided in '{ConfigurationKeys.AgentUri}' is not valid. It won't be taken into account to send traces.");


Suggested change

ValidationWarnings.Add($"The provided Uri: ${traceAgentUrl} provided in '{ConfigurationKeys.AgentUri}' is not valid. It won't be taken into account to send traces.");

ValidationWarnings.Add($"The Uri '${traceAgentUrl}' provided in '{ConfigurationKeys.AgentUri}' is not valid. It won't be taken into account to send traces.");

andrewlock · 2022-04-11T12:54:29Z

+            var traceAgentUrl = source?.GetString(ConfigurationKeys.AgentUri);
+            if (!string.IsNullOrWhiteSpace(traceAgentUrl))
+            {
+                if (!Uri.TryCreate(traceAgentUrl, UriKind.RelativeOrAbsolute, out var uri))


nit: could we invert the if here?

andrewlock · 2022-04-11T13:09:40Z

        public int PartialFlushMinSpans { get; set; }
        public string TracesPipeName { get; set; }
        public int TracesPipeTimeoutMs { get; set; }
-        public string TracesUnixDomainSocketPath { get; set; }


This shouldn't have gone I don't think 🤔

This is correct, TracesUnixDomainSocketPath got removed from ExporterSettings here but it is still present on the ImmutableExporterSettings type if you expand below

This was removed intentionally as it had been added only for .NET. (cf PR description)
This is the reason why Lucas rightfully asked me to document these settings (cf https://github.com/DataDog/architecture/pull/1000)

zacharycmontoya · 2022-04-11T21:16:54Z

            };

-            var tracerSettings = new TracerSettings(new NameValueConfigurationSource(settings));
+            var tracerSettings = new ImmutableTracerSettings(new TracerSettings(new NameValueConfigurationSource(settings)));


I'm a little on the fence, but I feel like this test belongs in ImmutableExporterSettingsTests since that is where the replacement of localhost occurs

zacharycmontoya · 2022-04-11T21:24:06Z

+        [Fact]
+        public void Traces_SocketFilesExist_ExplicitTraceAgentPort_UsesDefaultHttp()
+        {
+            // AgentPort isn't settable directly, so goinng a config source as a user would


Suggested change

// AgentPort isn't settable directly, so goinng a config source as a user would

// AgentPort isn't settable directly, so setting via a config source as a user would

zacharycmontoya · 2022-04-11T21:26:31Z

+            var immutableExporterSettings = Setup(FileExistsMock(ExporterSettings.DefaultMetricsUnixDomainSocket), new ExporterSettings());
+            Assert.Equal(expected: MetricsTransportType.UDS, actual: immutableExporterSettings.MetricsTransport);
+            Assert.Equal(expected: ExporterSettings.DefaultMetricsUnixDomainSocket, actual: immutableExporterSettings.MetricsUnixDomainSocketPath);
+            CheckDefaultValues(immutableExporterSettings, "MetricsTransport", "MetricsUnixDomainSocketPath", "DogStatsdPort");


Should we assert DogStatsdPort to be 0 in this instance?

zacharycmontoya

LGTM with some minor comments

andrewlock reviewed Feb 17, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Configuration/ExporterSettings.cs Outdated

lucaspimentel reviewed Feb 17, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Configuration/ExporterSettings.cs Outdated

lucaspimentel reviewed Feb 17, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Configuration/ExporterSettings.cs Outdated

lucaspimentel reviewed Feb 17, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Configuration/ExporterSettings.cs Outdated

lucaspimentel reviewed Feb 17, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.Tests/Configuration/ExporterSettingsTests.cs Outdated

pierotibou changed the title ~~[WIP] Review Exporter Settings~~ Review Exporter Settings Feb 19, 2022

pierotibou marked this pull request as ready for review February 19, 2022 00:09

pierotibou requested a review from a team as a code owner February 19, 2022 00:09

lucaspimentel reviewed Feb 23, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Configuration/ExporterSettings.cs Outdated

lucaspimentel reviewed Feb 23, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Configuration/ExporterSettings.cs

pierotibou added 9 commits March 9, 2022 14:23

Change ExporterSettings to build settings by order of precedence

c3c8c69

Add the URL as the first parameter to consider Change the tests as well to order them by order of precedence

Check Uri before creation and allow fallbacks

bc0ca8d

There's one use case that still throws, not sure what to do yet

Move Settings construction to ImmutableSettings

1baf076

Adds a builder to do it to keep the object immutable Changes the tests Adapted the checkAgentCommand

Complete Immutable Tests

1942d6e

... by checking default values all the time

Switch logs to validation warnings

f91d5a3

Based on the same system as used in DirectLogsSubmissionsSettings

And clear a few string potentially

84ce58f

and a last fix

18c0596

as ususal forgot a file

613ce91

Remove DD_APM_RECEIVER_SOCKET_PARAM

e445c99

Note that it is a breaking change

pierotibou force-pushed the pierre/uds-config branch from a100c5c to e445c99 Compare March 9, 2022 13:28

pierotibou added 2 commits March 11, 2022 15:53

Remove CustomTcpProvider

ed3f9ab

it was used only in tests and it doesn't seem we are going further down this road

fix cli tests build and add a new test

0b54a17

lucaspimentel approved these changes Apr 5, 2022

View reviewed changes

lucaspimentel added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Apr 5, 2022

Merge branch 'master' into pierre/uds-config

e813d5a

andrewlock reviewed Apr 11, 2022

View reviewed changes

zacharycmontoya reviewed Apr 11, 2022

View reviewed changes

zacharycmontoya approved these changes Apr 11, 2022

View reviewed changes

pierotibou mentioned this pull request Apr 14, 2022

[Tracer] Rework exporter settings #2686

Closed

2 tasks

andrewlock mentioned this pull request Apr 20, 2022

If DirectLogSubmission is enabled, enable Logs Injection by default #2697

Merged

pierotibou mentioned this pull request Apr 27, 2022

[Tracer] Fix precedence in ExporterSettings #2737

Merged

pierotibou closed this May 2, 2022

	ValidationWarnings.Add($"The provided Uri: ${traceAgentUrl} provided in '{ConfigurationKeys.AgentUri}' is not valid. It won't be taken into account to send traces.");
	ValidationWarnings.Add($"The Uri '${traceAgentUrl}' provided in '{ConfigurationKeys.AgentUri}' is not valid. It won't be taken into account to send traces.");

	// AgentPort isn't settable directly, so goinng a config source as a user would
	// AgentPort isn't settable directly, so setting via a config source as a user would

Uh oh!

Conversation

pierotibou commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Testing

Uh oh!

andrewlock commented Feb 17, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewlock Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

pierotibou Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pierotibou commented Feb 17, 2022 •

edited

Loading

pierotibou Apr 13, 2022 •

edited

Loading