Skip to content

[CIVisibility] - Move the CI Visibility check to native#3196

Merged
tonyredondo merged 3 commits into
masterfrom
tony/ci-visibility-mode-check
Sep 12, 2022
Merged

[CIVisibility] - Move the CI Visibility check to native#3196
tonyredondo merged 3 commits into
masterfrom
tony/ci-visibility-mode-check

Conversation

@tonyredondo

@tonyredondo tonyredondo commented Sep 9, 2022

Copy link
Copy Markdown
Member

Summary of changes

This PR moves the CI Visibility check to native and avoid loading the profiler on process we don't want to instrument.

Reason for change

The current managed implementation requires the CLR profiler to be loaded, the loader to be injected and then disabled by the PInvoke call.

In this implementation we avoid to load the CLR profiler at all.

Implementation details

Same as managed implementation but in native.

@tonyredondo tonyredondo added area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) type:performance Performance, speed, latency, resource usage (CPU, memory) area:ci-visibility labels Sep 9, 2022
@tonyredondo tonyredondo self-assigned this Sep 9, 2022
@tonyredondo
tonyredondo marked this pull request as ready for review September 9, 2022 12:44
@tonyredondo
tonyredondo requested review from a team as code owners September 9, 2022 12:44
@andrewlock

This comment has been minimized.

Sign in to view
dromanol
dromanol approved these changes Sep 9, 2022
View reviewed changes

@dromanol dromanol left a comment

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.

LGTM 👍

Comment thread tracer/src/Datadog.Tracer.Native/cor_profiler.cpp
@andrewlock

This comment has been minimized.

Sign in to view
gleocadie
gleocadie reviewed Sep 9, 2022
View reviewed changes
Comment thread tracer/src/Datadog.Tracer.Native/cor_profiler.cpp Outdated
gleocadie
gleocadie approved these changes Sep 9, 2022
View reviewed changes

@gleocadie gleocadie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit comment, otherwise LGTM

@andrewlock

andrewlock commented Sep 12, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3196 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.152
  • 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 730μs 387ns 1.5μs 0.363 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 455μs 333ns 1.29μs 0 0 0 2.58 KB
#3196 WriteAndFlushEnrichedTraces net472 721μs 437ns 1.69μs 0.359 0 0 3.18 KB
#3196 WriteAndFlushEnrichedTraces netcoreapp3.1 473μs 273ns 1.06μs 0 0 0 2.59 KB
Benchmarks.Trace.AppSecBodyBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #3196

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleSimpleBody‑net472 1.152 213.76 185.48

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 214ns 0.194ns 0.726ns 0.0675 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 238ns 0.355ns 1.33ns 0.00577 0 0 424 B
master AllCycleMoreComplexBody net472 184ns 0.151ns 0.586ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 231ns 0.282ns 1.02ns 0.00546 0 0 400 B
master BodyExtractorSimpleBody net472 272ns 0.276ns 1.07ns 0.0574 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 232ns 0.248ns 0.894ns 0.00384 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.2μs 4.52ns 16.3ns 1.2 0.0214 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.1μs 13.5ns 52.2ns 0.0903 0 0 6.75 KB
#3196 AllCycleSimpleBody net472 186ns 0.119ns 0.445ns 0.0676 0 0 425 B
#3196 AllCycleSimpleBody netcoreapp3.1 236ns 0.232ns 0.869ns 0.00585 0 0 424 B
#3196 AllCycleMoreComplexBody net472 195ns 0.316ns 1.14ns 0.0637 0 0 401 B
#3196 AllCycleMoreComplexBody netcoreapp3.1 232ns 0.351ns 1.36ns 0.00537 0 0 400 B
#3196 BodyExtractorSimpleBody net472 260ns 0.175ns 0.607ns 0.0573 0 0 361 B
#3196 BodyExtractorSimpleBody netcoreapp3.1 228ns 0.285ns 1.1ns 0.00379 0 0 272 B
#3196 BodyExtractorMoreComplexBody net472 14.5μs 12.1ns 46.9ns 1.21 0.0218 0 7.62 KB
#3196 BodyExtractorMoreComplexBody netcoreapp3.1 12μs 14.7ns 55.1ns 0.0895 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 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 186μs 306ns 1.19μs 0.279 0 0 20.57 KB
#3196 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3196 SendRequest netcoreapp3.1 184μs 315ns 1.22μs 0.185 0 0 20.57 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.85μs 1.09ns 4.21ns 0.15 0 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.46μs 0.875ns 3.39ns 0.0125 0 0 936 B
#3196 ExecuteNonQuery net472 1.81μs 0.958ns 3.71ns 0.15 0.000908 0 947 B
#3196 ExecuteNonQuery netcoreapp3.1 1.44μs 0.327ns 1.22ns 0.0129 0 0 936 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.7μs 1.46ns 5.48ns 0.184 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.52μs 1.15ns 4.45ns 0.0144 0 0 1.1 KB
master CallElasticsearchAsync net472 2.77μs 1.49ns 5.76ns 0.204 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.59μs 0.559ns 2.16ns 0.0166 0 0 1.22 KB
#3196 CallElasticsearch net472 2.64μs 2.06ns 7.97ns 0.184 0.00133 0 1.16 KB
#3196 CallElasticsearch netcoreapp3.1 1.56μs 1.35ns 5.21ns 0.0147 0 0 1.1 KB
#3196 CallElasticsearchAsync net472 2.74μs 2.44ns 9.47ns 0.205 0 0 1.29 KB
#3196 CallElasticsearchAsync netcoreapp3.1 1.58μs 0.791ns 2.96ns 0.0166 0 0 1.22 KB
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 2.76μs 6.04ns 21.8ns 0.225 0.00138 0 1.42 KB
master ExecuteAsync netcoreapp3.1 1.67μs 5.79ns 21.7ns 0.0182 0.000865 0 1.34 KB
#3196 ExecuteAsync net472 2.66μs 6.24ns 24.2ns 0.226 0 0 1.42 KB
#3196 ExecuteAsync netcoreapp3.1 1.65μs 3.21ns 12.4ns 0.0182 0 0 1.34 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 5.81μs 19.2ns 74.2ns 0.439 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.62μs 9.54ns 35.7ns 0.0348 0 0 2.6 KB
#3196 SendAsync net472 5.72μs 17.7ns 68.6ns 0.439 0 0 2.77 KB
#3196 SendAsync netcoreapp3.1 3.54μs 5.86ns 21.1ns 0.0354 0 0 2.6 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.18μs 2.37ns 9.17ns 0.287 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.57μs 1.05ns 4.05ns 0.0256 0 0 1.85 KB
#3196 EnrichedLog net472 3.2μs 3.18ns 11.9ns 0.288 0 0 1.81 KB
#3196 EnrichedLog netcoreapp3.1 2.46μs 1.25ns 4.86ns 0.0247 0 0 1.85 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 153μs 234ns 906ns 0.695 0.232 0 4.66 KB
master EnrichedLog netcoreapp3.1 117μs 119ns 460ns 0.0584 0 0 4.5 KB
#3196 EnrichedLog net472 150μs 96ns 359ns 0.672 0.224 0 4.65 KB
#3196 EnrichedLog netcoreapp3.1 115μs 267ns 1.03μs 0.0572 0 0 4.5 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 5.63μs 13.3ns 51.7ns 0.568 0.00276 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.29μs 6.92ns 25ns 0.0537 0 0 3.91 KB
#3196 EnrichedLog net472 5.64μs 4.71ns 17.6ns 0.569 0.00282 0 3.59 KB
#3196 EnrichedLog netcoreapp3.1 4.24μs 7.03ns 27.2ns 0.0531 0 0 3.91 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.29μs 3.5ns 13.5ns 0.217 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.8μs 0.677ns 2.53ns 0.018 0 0 1.32 KB
#3196 SendReceive net472 2.3μs 1.85ns 6.94ns 0.217 0 0 1.37 KB
#3196 SendReceive netcoreapp3.1 1.85μs 0.776ns 2.9ns 0.0176 0 0 1.32 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 5.07μs 1.88ns 7.03ns 0.354 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.33μs 2.25ns 8.71ns 0.0237 0 0 1.8 KB
#3196 EnrichedLog net472 5.03μs 2.53ns 9.8ns 0.355 0.0025 0 2.23 KB
#3196 EnrichedLog netcoreapp3.1 4.18μs 2.48ns 9.61ns 0.0231 0 0 1.8 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 1.21μs 0.776ns 2.9ns 0.129 0 0 810 B
master StartFinishSpan netcoreapp3.1 922ns 0.597ns 2.31ns 0.0102 0 0 760 B
master StartFinishScope net472 1.37μs 1.81ns 6.99ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.07μs 0.587ns 2.12ns 0.0119 0 0 880 B
#3196 StartFinishSpan net472 1.19μs 0.724ns 2.8ns 0.129 0 0 810 B
#3196 StartFinishSpan netcoreapp3.1 913ns 0.393ns 1.52ns 0.01 0 0 760 B
#3196 StartFinishScope net472 1.41μs 0.925ns 3.58ns 0.141 0 0 891 B
#3196 StartFinishScope netcoreapp3.1 1.09μs 0.501ns 1.88ns 0.012 0 0 880 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.5μs 0.784ns 3.04ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.13μs 0.754ns 2.92ns 0.012 0 0 880 B
#3196 RunOnMethodBegin net472 1.49μs 1.11ns 4.29ns 0.141 0 0 891 B
#3196 RunOnMethodBegin netcoreapp3.1 1.14μs 0.47ns 1.82ns 0.012 0 0 880 B

@tonyredondo
tonyredondo merged commit 5bbf2b2 into master Sep 12, 2022
@tonyredondo
tonyredondo deleted the tony/ci-visibility-mode-check branch September 12, 2022 12:45
@github-actions github-actions Bot added this to the vNext milestone Sep 12, 2022
@andrewlock

andrewlock commented Sep 12, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

✔️ Merging #3196 into master will will increase line coverage by 1%
✔️ Merging #3196 into master will will increase branch coverage by 1%
⛔ Merging #3196 into master will will increase complexity by 90

master #3196 Change
Lines 17696 / 24369 17934 / 24513
Lines % 73% 73% 1% ✔️
Branches 10398 / 14968 10579 / 15050
Branches % 69% 70% 1% ✔️
Complexity 16169 16259 90 ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3196 Change
Lines % 73% 73% 1% ✔️
Branches % 69% 70% 1% ✔️
Complexity 16169 16259 90 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Ci.GitInfo -17% ⛔ -11% ⛔ 0 ✔️
Datadog.Trace.Agent.Transports.HttpStreamRequestFactory -12% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.Sink.AgentBatchUploadApi -8% ⛔ 0% ✔️ 2 ⛔
Datadog.Trace.ClrProfiler.NativeMethods -6% ⛔ -2% ⚠️ 0 ✔️
Datadog.Trace.TracerManagerFactory 3% ✔️ 6% ✔️ 2 ⛔
Datadog.Trace.Logging.DatadogSerilogLogger 4% ✔️ 6% ✔️ 0 ✔️
Datadog.Trace.Propagators.SpanContextPropagator 8% ✔️ 6% ✔️ 0 ✔️
Datadog.Trace.Agent.StatsAggregator 11% ✔️ 15% ✔️ -2 ✔️
Datadog.Trace.Processors.ObfuscatorTraceProcessor 12% ✔️ 15% ✔️ 0 ✔️
Datadog.Trace.Telemetry.Transports.JsonTelemetryTransport 12% ✔️ 17% ✔️ 0 ✔️
...And 8 more

The following classes were added in #3196:

File Line coverage Branch coverage Complexity
Datadog.Trace.Agent.DiscoveryService.AgentConfiguration 100% 100% 1
Datadog.Trace.ClrProfiler.AutoInstrumentation.Process.EnvironmentVariablesScrubber 100% 100% 11
Datadog.Trace.ClrProfiler.AutoInstrumentation.Process.ProcessStartCommon 85% 80% 11
Datadog.Trace.ClrProfiler.AutoInstrumentation.Process.ProcessStartIntegration 80% 50% 3
Datadog.Trace.Tagging.ProcessCommandStartTags 100% 100% 1
...And 1 more

View the full reports for further details:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gleocadie gleocadie gleocadie approved these changes
@dromanol dromanol dromanol approved these changes

Assignees

@tonyredondo tonyredondo

Labels

area:ci-visibility area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) type:performance Performance, speed, latency, resource usage (CPU, memory)

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

4 participants

@tonyredondo @andrewlock @gleocadie @dromanol