Skip to content

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

Merged
andrewlock merged 1 commit into
masterfrom
andrew/enable-logs-injection-when-using-direct-log-submission
Apr 20, 2022
Merged

If DirectLogSubmission is enabled, enable Logs Injection by default#2697
andrewlock merged 1 commit into
masterfrom
andrew/enable-logs-injection-when-using-direct-log-submission

Conversation

@andrewlock

@andrewlock andrewlock commented Apr 19, 2022

Copy link
Copy Markdown
Member

Summary of changes

  • Enables logs injection by default when direct log submission is enabled

Reason for change

Improve onboarding for direct log submission

Implementation details

Previously, users had to remember to set DD_LOGS_INJECTION as well as DD_LOGS_DIRECT_SUBMISSION_INTEGRATIONS to have their traces correlated. There are vanishingly few cases where you would want to enable DD_LOGS_DIRECT_SUBMISSION_INTEGRATIONS but not enable DD_LOGS_INJECTION.

The only scenario I could think of is where a user is not using tracing, but they want to use direct log submission to send logs and also output to a different sink, where enabling logs injection would break their output. This seems an incredibly niche scenario, and one that can be solved by explicitly setting DD_LOGS_INJECTION=0 if required.

Unfortunately, LogsInjectionEnabled is one of the properties we allow to be set in code, so it makes implementing this slightly trickier, but I think the work around in this PR is sufficient.

Test coverage

Unit tests for the setting value

@andrewlock andrewlock added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Apr 19, 2022
@andrewlock
andrewlock requested a review from a team as a code owner April 19, 2022 10:49
andrewlock
andrewlock commented Apr 19, 2022
View reviewed changes
Comment thread tracer/src/Datadog.Trace/Configuration/TracerSettings.cs
public bool LogsInjectionEnabled { get; set; }
public bool LogsInjectionEnabled
{
get => LogSubmissionSettings?.LogsInjectionEnabled ?? false;

@andrewlock andrewlock Apr 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This isn't quite right, as it doesn't account for the fact that if LogSubmissionSettings is ultimately enabled, then the default is true, not false. Unfortunately, it's not easy to know whether direct log submission will be enabled at this point, as we don't decide until we get to ImmutableTracerSettings (where we create the final ImmutableDirectLogSubmissionSettings).

I think this is, again, an acceptable trade-off, as it seems unlikely that users are calling the getter here? The alternative is to remove DirectLogSubmissionSettings entirely, and create the ImmutableDirectLogSubmissionSettings in the TracerSettings constructor?

@zacharycmontoya zacharycmontoya Apr 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems perfectly fine to me. The TracerSettings object is basically an input into the Tracer configuration so the resulting ImmutableTracerSettings is the object that has the true result

@andrewlock andrewlock Apr 20, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The TracerSettings object is basically an input into the Tracer configuration so the resulting ImmutableTracerSettings is the object that has the true result

I totally agree that this is what it should be, though I worry that customers may in general be not treating the TracerSettings as "set only". This has come up with @pierotibou recently around what assumptions we can make refactoring ExporterSettings in #2455 and #2686. I think it's ok here, but there's definitely a question as to whether we should make that contract explicit...

@pierotibou pierotibou Apr 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe it's acceptable given the current model (and we should change this model /rant)

@andrewlock

andrewlock commented Apr 19, 2022

Copy link
Copy Markdown
Member Author

Code Coverage Report 📊

⚠️ Merging #2697 into master will will decrease line coverage by 1%
⚠️ Merging #2697 into master will will decrease branch coverage by 1%
⛔ Merging #2697 into master will will increase complexity by 276

master #2697 Change
Lines 13550 / 18651 13674 / 18980
Lines % 73% 72% -1% ⚠️
Branches 7822 / 11131 7943 / 11444
Branches % 70% 69% -1% ⚠️
Complexity 12416 12692 276 ⛔

View the full report for further details:

Datadog.Trace Breakdown ⚠️

master #2697 Change
Lines % 73% 72% -1% ⚠️
Branches % 70% 69% -1% ⚠️
Complexity 12416 12692 276 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.AppSec.Waf.Initialization.WafConfigurator -23% ⛔ -17% ⛔ 40 ⛔
Datadog.Trace.Logging.DirectSubmission.Sink.LogsApi -22% ⛔ -26% ⛔ 34 ⛔
Datadog.Trace.AppSec.SecuritySettings -19% ⛔ -18% ⛔ 46 ⛔
Datadog.Trace.AppSec.Waf.Waf -16% ⛔ -12% ⛔ 9 ⛔
Datadog.Trace.TracerManager -13% ⛔ -21% ⛔ 33 ⛔
Datadog.Trace.Ci.Processors.OriginTagTraceProcessor -9% ⛔ -17% ⛔ 0 ✔️
Datadog.Trace.Agent.Transports.HttpClientRequestFactory -9% ⛔ -6% ⛔ 11 ⛔
Datadog.Trace.Telemetry.ConfigurationTelemetryCollector -8% ⛔ -10% ⛔ 2 ⛔
Datadog.Trace.Configuration.ImmutableTracerSettings -7% ⛔ -5% ⛔ 21 ⛔
Datadog.Trace.Configuration.TracerSettings -6% ⛔ -8% ⛔ 37 ⛔
...And 4 more

The following classes were added in #2697:

File Line coverage Branch coverage Complexity
Datadog.Trace.Agent.Transports.UdsClientRequestFactory 100% 100% 8

View the full reports for further details:

@andrewlock

andrewlock commented Apr 19, 2022

Copy link
Copy Markdown
Member Author

Benchmarks Report 🐌

Benchmarks for #2697 compared to master:

  • 2 benchmarks are slower, with geometric mean 1.219
  • 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 Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 1.03ms 5.33μs 24.4μs 0 0 0 3.16 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 718μs 3.5μs 18.5μs 0 0 0 2.57 KB
#2697 WriteAndFlushEnrichedTraces net472 1.02ms 5.04μs 25.7μs 0 0 0 3.16 KB
#2697 WriteAndFlushEnrichedTraces netcoreapp3.1 714μs 3.42μs 15.3μs 0 0 0 2.57 KB
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 313ns 1.59ns 6.93ns 0.066 0 0 417 B
master AllCycleSimpleBody netcoreapp3.1 415ns 1.55ns 6.01ns 0.0058 0 0 416 B
master AllCycleMoreComplexBody net472 307ns 1.38ns 5.18ns 0.0622 0 0 393 B
master AllCycleMoreComplexBody netcoreapp3.1 396ns 1.29ns 4.84ns 0.00535 0 0 392 B
master BodyExtractorSimpleBody net472 454ns 2.31ns 11.1ns 0.0569 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 447ns 1.97ns 7.62ns 0.00375 0 0 272 B
master BodyExtractorMoreComplexBody net472 24μs 87.5ns 316ns 1.18 0.0122 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 21μs 75ns 291ns 0.0952 0 0 6.75 KB
#2697 AllCycleSimpleBody net472 331ns 1.87ns 13.1ns 0.066 0 0 417 B
#2697 AllCycleSimpleBody netcoreapp3.1 414ns 2ns 8.24ns 0.00585 0 0 416 B
#2697 AllCycleMoreComplexBody net472 332ns 1.7ns 7.6ns 0.0622 0 0 393 B
#2697 AllCycleMoreComplexBody netcoreapp3.1 418ns 2.11ns 9.66ns 0.00534 0 0 392 B
#2697 BodyExtractorSimpleBody net472 449ns 2.49ns 14.9ns 0.0569 0 0 361 B
#2697 BodyExtractorSimpleBody netcoreapp3.1 467ns 2.67ns 19.4ns 0.00382 0 0 272 B
#2697 BodyExtractorMoreComplexBody net472 25μs 129ns 606ns 1.19 0.0121 0 7.62 KB
#2697 BodyExtractorMoreComplexBody netcoreapp3.1 22.2μs 128ns 1.02μs 0.0964 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
master SendRequest netcoreapp3.1 295μs 1.4μs 5.44μs 0.293 0 0 19.74 KB
#2697 SendRequest net472 0.00262ns 0.00184ns 0.0115ns 0 0 0 0 b
#2697 SendRequest netcoreapp3.1 310μs 1.55μs 6.95μs 0.146 0 0 19.74 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net472 1.7μs 8.34ns 33.4ns 0.0941 0 0 594 B
master ExecuteNonQuery netcoreapp3.1 1.6μs 9.29ns 73.7ns 0.00849 0 0 632 B
#2697 ExecuteNonQuery net472 1.76μs 10.1ns 72ns 0.0936 0 0 594 B
#2697 ExecuteNonQuery netcoreapp3.1 1.56μs 8.12ns 40.6ns 0.00836 0 0 632 B
Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2697

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net472 1.183 2,506.26 2,964.85

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net472 2.51μs 12.2ns 48.8ns 0.127 0 0 803 B
master CallElasticsearch netcoreapp3.1 1.68μs 6.15ns 23ns 0.0109 0 0 792 B
master CallElasticsearchAsync net472 2.76μs 11.8ns 45.7ns 0.148 0.00135 0 939 B
master CallElasticsearchAsync netcoreapp3.1 2.11μs 10.9ns 50ns 0.0124 0 0 912 B
#2697 CallElasticsearch net472 2.98μs 14.1ns 62.9ns 0.127 0 0 803 B
#2697 CallElasticsearch netcoreapp3.1 1.83μs 10.6ns 88.9ns 0.0113 0 0 792 B
#2697 CallElasticsearchAsync net472 2.79μs 13.3ns 59.3ns 0.149 0.00137 0 939 B
#2697 CallElasticsearchAsync netcoreapp3.1 1.9μs 10.2ns 54.2ns 0.0131 0 0 912 B
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net472 3μs 14.2ns 60.3ns 0.168 0.00147 0 1.06 KB
master ExecuteAsync netcoreapp3.1 2μs 11.3ns 75.1ns 0.0143 0 0 1.03 KB
#2697 ExecuteAsync net472 3.26μs 19ns 159ns 0.165 0 0 1.06 KB
#2697 ExecuteAsync netcoreapp3.1 2.09μs 10.9ns 54.5ns 0.0139 0 0 1.03 KB
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net472 6.79μs 33.6ns 138ns 0.361 0 0 2.28 KB
master SendAsync netcoreapp3.1 4.68μs 16.8ns 65.2ns 0.0301 0 0 2.21 KB
#2697 SendAsync net472 7.25μs 39.5ns 230ns 0.359 0 0 2.28 KB
#2697 SendAsync netcoreapp3.1 5.03μs 18.5ns 69.2ns 0.0296 0 0 2.21 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 3.59μs 19.8ns 121ns 0.227 0 0 1.45 KB
master EnrichedLog netcoreapp3.1 3.15μs 17.4ns 99.9ns 0.0213 0 0 1.53 KB
#2697 EnrichedLog net472 3.38μs 17.2ns 82.5ns 0.228 0 0 1.45 KB
#2697 EnrichedLog netcoreapp3.1 3.16μs 14.5ns 56.3ns 0.0213 0 0 1.53 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 284μs 1.33μs 4.97μs 0.441 0.147 0 4.33 KB
master EnrichedLog netcoreapp3.1 227μs 745ns 2.79μs 0 0 0 4.21 KB
#2697 EnrichedLog net472 296μs 1.5μs 6.52μs 0.449 0.15 0 4.33 KB
#2697 EnrichedLog netcoreapp3.1 237μs 1.28μs 7.34μs 0 0 0 4.21 KB
Benchmarks.Trace.NLogBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2697

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.NLogBenchmark.EnrichedLog‑net472 1.257 7,306.12 9,180.48

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 7.32μs 30.3ns 109ns 0.506 0 0 3.23 KB
master EnrichedLog netcoreapp3.1 6.4μs 30.2ns 117ns 0.0507 0 0 3.6 KB
#2697 EnrichedLog net472 9.17μs 52.1ns 368ns 0.505 0 0 3.23 KB
#2697 EnrichedLog netcoreapp3.1 6.51μs 33.7ns 162ns 0.0475 0 0 3.6 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net472 2.42μs 9.33ns 37.3ns 0.16 0.00118 0 1.01 KB
master SendReceive netcoreapp3.1 1.96μs 6.33ns 24.5ns 0.0136 0 0 1.01 KB
#2697 SendReceive net472 2.48μs 13.7ns 84.5ns 0.16 0.00128 0 1.01 KB
#2697 SendReceive netcoreapp3.1 2.22μs 12.6ns 89.9ns 0.0137 0 0 1.01 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 6.61μs 37.2ns 244ns 0.292 0 0 1.87 KB
master EnrichedLog netcoreapp3.1 5.19μs 22.7ns 84.8ns 0.0207 0 0 1.49 KB
#2697 EnrichedLog net472 6.62μs 25.2ns 97.5ns 0.291 0 0 1.87 KB
#2697 EnrichedLog netcoreapp3.1 5.32μs 30.1ns 204ns 0.0185 0 0 1.49 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net472 924ns 4.42ns 18.7ns 0.0719 0 0 457 B
master StartFinishSpan netcoreapp3.1 932ns 5.31ns 39.1ns 0.00618 0 0 456 B
master StartFinishScope net472 1.06μs 5.42ns 24.8ns 0.0845 0 0 538 B
master StartFinishScope netcoreapp3.1 1.08μs 6.26ns 53.5ns 0.00805 0 0 576 B
#2697 StartFinishSpan net472 1.03μs 3.96ns 14.3ns 0.0717 0 0 457 B
#2697 StartFinishSpan netcoreapp3.1 935ns 4.94ns 38.6ns 0.00624 0 0 456 B
#2697 StartFinishScope net472 1.19μs 6.65ns 40.4ns 0.084 0 0 538 B
#2697 StartFinishScope netcoreapp3.1 1.08μs 6.13ns 42ns 0.00797 0 0 576 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net472 1.36μs 7.31ns 42ns 0.0843 0 0 538 B
master RunOnMethodBegin netcoreapp3.1 1.26μs 6.62ns 31.7ns 0.0075 0 0 576 B
#2697 RunOnMethodBegin net472 1.3μs 6.74ns 32.3ns 0.0843 0 0 538 B
#2697 RunOnMethodBegin netcoreapp3.1 1.16μs 5.04ns 19.5ns 0.00789 0 0 576 B

@andrewlock
andrewlock merged commit d7736ae into master Apr 20, 2022
@andrewlock
andrewlock deleted the andrew/enable-logs-injection-when-using-direct-log-submission branch April 20, 2022 09:55
@github-actions github-actions Bot added this to the vNext milestone Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pierotibou pierotibou pierotibou approved these changes
@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes

Assignees

No one assigned

Labels

area:direct-log-submission area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations)

Projects

None yet

Milestone

2.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@andrewlock @zacharycmontoya @pierotibou