[ASM] only collect ip for appsec by robertpi · Pull Request #3379 · DataDog/dd-trace-dotnet

robertpi · 2022-10-21T12:29:39Z

Summary of changes

Collect ip only when ASM is enabled.

Reason for change

Ip collection is now considered to be sensitive information that should not be collected, unless ASM (app security) is enabled, ip collecting being required for protection purposes.

Implementation details

Surrounding with condition if appsec is enabled all the ip collecting places.

Test coverage

All snapshosts are impacted, with removing ip collection tags.
Span metadata md is impacted as both fields http.client_ip and network.client.ip are now optional (only when appsec is on).

Other details

Message of multiple ip headers found only at debug level.

andrewlock

LGTM, thanks for fixing the log :) one question about whether this should still be controllable, or if it's required when ASM is enabled

github-actions · 2022-10-24T13:35:22Z

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

223 occurrences of :

-      http.client_ip: 127.0.0.1,
[...]
-      network.client.ip: ::1,

808 occurrences of :

-      http.client_ip: 127.0.0.1,

522 occurrences of :

-      network.client.ip: ::1,

117 occurrences of :

-      http.client_ip: 127.0.0.1,
[...]
-      network.client.ip: 127.0.0.1,

289 occurrences of :

-      network.client.ip: 127.0.0.1,

3 occurrences of :

-      http.client_ip: 86.242.244.246,

andrewlock · 2022-10-24T14:37:46Z

Benchmarks Report 🐌

Benchmarks for #3379 compared to master:

1 benchmarks are faster, with geometric mean 1.144
1 benchmarks have fewer 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	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	803μs	302ns	1.04μs	0.401	3.22 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	544μs	193ns	746ns	0	2.63 KB
#3379	`WriteAndFlushEnrichedTraces`	net472	807μs	526ns	2.04μs	0.401	3.22 KB
#3379	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	531μs	185ns	715ns	0	2.63 KB

Benchmarks.Trace.AppSecBodyBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #3379

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.AppSecBodyBenchmark.BodyExtractorSimpleBody‑netcoreapp3.1	1.144	262.12	229.04

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	181ns	0.0508ns	0.183ns	0.0701	9.18E-05	441 B
master	`AllCycleSimpleBody`	netcoreapp3.1	243ns	0.332ns	1.29ns	0.00603	0	440 B
master	`AllCycleMoreComplexBody`	net472	181ns	0.0299ns	0.108ns	0.0662	0	417 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	238ns	0.158ns	0.611ns	0.00562	0	416 B
master	`BodyExtractorSimpleBody`	net472	297ns	0.301ns	1.16ns	0.0574	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	262ns	0.195ns	0.702ns	0.00368	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	16.1μs	10.5ns	39.2ns	1.21	0.016	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	13.5μs	80.2ns	744ns	0.0898	0	6.75 KB
#3379	`AllCycleSimpleBody`	net472	182ns	0.086ns	0.322ns	0.0701	9.14E-05	441 B
#3379	`AllCycleSimpleBody`	netcoreapp3.1	239ns	0.1ns	0.375ns	0.00598	0	440 B
#3379	`AllCycleMoreComplexBody`	net472	181ns	0.0482ns	0.187ns	0.0663	0	417 B
#3379	`AllCycleMoreComplexBody`	netcoreapp3.1	237ns	0.201ns	0.777ns	0.00563	0	416 B
#3379	`BodyExtractorSimpleBody`	net472	283ns	0.228ns	0.853ns	0.0574	0	361 B
#3379	`BodyExtractorSimpleBody`	netcoreapp3.1	229ns	0.168ns	0.63ns	0.00371	0	272 B
#3379	`BodyExtractorMoreComplexBody`	net472	15.7μs	9.99ns	38.7ns	1.21	0.0158	7.62 KB
#3379	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.3μs	5.49ns	20.5ns	0.0862	0	6.75 KB

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

Fewer allocations 🎉 in #3379

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.AspNetCoreBenchmark.SendRequest‑netcoreapp3.1	20.66 KB	20.44 KB	-224 B	-1.08%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	179μs	146ns	546ns	0.268	20.66 KB
#3379	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3379	`SendRequest`	netcoreapp3.1	178μs	250ns	970ns	0.176	20.44 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.81μs	0.589ns	2.28ns	0.158	0.000909	995 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.4μs	0.402ns	1.51ns	0.0133	0	984 B
#3379	`ExecuteNonQuery`	net472	1.75μs	0.376ns	1.46ns	0.157	0.000889	995 B
#3379	`ExecuteNonQuery`	netcoreapp3.1	1.42μs	0.345ns	1.29ns	0.0135	0	984 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.46μs	1.7ns	6.6ns	0.191	1.2 KB
master	`CallElasticsearch`	netcoreapp3.1	1.6μs	0.722ns	2.8ns	0.0151	1.14 KB
master	`CallElasticsearchAsync`	net472	2.66μs	1.36ns	4.91ns	0.213	1.34 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.64μs	0.418ns	1.45ns	0.0174	1.26 KB
#3379	`CallElasticsearch`	net472	2.38μs	1.77ns	6.61ns	0.191	1.2 KB
#3379	`CallElasticsearch`	netcoreapp3.1	1.51μs	0.805ns	3.01ns	0.0158	1.14 KB
#3379	`CallElasticsearchAsync`	net472	2.71μs	2.26ns	8.15ns	0.212	1.34 KB
#3379	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	4ns	15ns	0.0166	1.26 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.69μs	1.3ns	4.85ns	0.233	1.47 KB
master	`ExecuteAsync`	netcoreapp3.1	1.67μs	0.615ns	2.3ns	0.0183	1.39 KB
#3379	`ExecuteAsync`	net472	2.62μs	1.91ns	7.14ns	0.233	1.47 KB
#3379	`ExecuteAsync`	netcoreapp3.1	1.72μs	3.49ns	13.5ns	0.0188	1.39 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.88μs	2.95ns	11.4ns	0.447	2.82 KB
master	`SendAsync`	netcoreapp3.1	3.71μs	1.28ns	4.77ns	0.0353	2.65 KB
#3379	`SendAsync`	net472	5.96μs	8.64ns	31.1ns	0.446	2.82 KB
#3379	`SendAsync`	netcoreapp3.1	3.67μs	1.18ns	4.42ns	0.035	2.65 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.25μs	1.71ns	6.38ns	0.297	1.88 KB
master	`EnrichedLog`	netcoreapp3.1	2.45μs	1.58ns	5.71ns	0.0258	1.91 KB
#3379	`EnrichedLog`	net472	3.14μs	2.5ns	9.67ns	0.298	1.88 KB
#3379	`EnrichedLog`	netcoreapp3.1	2.51μs	1.31ns	4.91ns	0.0252	1.91 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	152μs	227ns	878ns	0.683	0.228	4.72 KB
master	`EnrichedLog`	netcoreapp3.1	120μs	185ns	715ns	0.0606	0	4.55 KB
#3379	`EnrichedLog`	net472	151μs	130ns	505ns	0.679	0.226	4.72 KB
#3379	`EnrichedLog`	netcoreapp3.1	119μs	153ns	591ns	0.0595	0	4.55 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.75μs	2.52ns	9.76ns	0.578	0.00287	3.65 KB
master	`EnrichedLog`	netcoreapp3.1	4.52μs	1.72ns	6.45ns	0.0523	0	3.98 KB
#3379	`EnrichedLog`	net472	5.69μs	2.06ns	7.44ns	0.58	0.00286	3.65 KB
#3379	`EnrichedLog`	netcoreapp3.1	4.53μs	1.93ns	7.21ns	0.0545	0	3.98 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.32μs	1.96ns	7.6ns	0.226	1.42 KB
master	`SendReceive`	netcoreapp3.1	1.82μs	1.15ns	4.32ns	0.0191	1.37 KB
#3379	`SendReceive`	net472	2.36μs	1.91ns	7.15ns	0.225	1.42 KB
#3379	`SendReceive`	netcoreapp3.1	1.68μs	0.837ns	3.13ns	0.0187	1.37 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	4.92μs	3.77ns	14.6ns	0.365	2.3 KB
master	`EnrichedLog`	netcoreapp3.1	4.44μs	1.21ns	4.38ns	0.0245	1.86 KB
#3379	`EnrichedLog`	net472	5μs	5.47ns	20.5ns	0.363	2.3 KB
#3379	`EnrichedLog`	netcoreapp3.1	4.42μs	2.34ns	8.74ns	0.0243	1.86 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.27μs	0.616ns	2.3ns	0.136	859 B
master	`StartFinishSpan`	netcoreapp3.1	981ns	0.726ns	2.62ns	0.0107	808 B
master	`StartFinishScope`	net472	1.38μs	0.491ns	1.84ns	0.149	939 B
master	`StartFinishScope`	netcoreapp3.1	1.04μs	0.566ns	2.19ns	0.0127	928 B
#3379	`StartFinishSpan`	net472	1.17μs	0.247ns	0.957ns	0.136	859 B
#3379	`StartFinishSpan`	netcoreapp3.1	954ns	0.416ns	1.5ns	0.011	808 B
#3379	`StartFinishScope`	net472	1.42μs	0.433ns	1.56ns	0.149	939 B
#3379	`StartFinishScope`	netcoreapp3.1	1.14μs	0.518ns	2.01ns	0.0124	928 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.41μs	0.218ns	0.786ns	0.149	939 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.12μs	0.405ns	1.57ns	0.0124	928 B
#3379	`RunOnMethodBegin`	net472	1.52μs	0.399ns	1.44ns	0.149	939 B
#3379	`RunOnMethodBegin`	netcoreapp3.1	1.13μs	0.932ns	3.36ns	0.0124	928 B

andrewlock · 2022-10-24T14:42:59Z

Code Coverage Report 📊

✔️ Merging #3379 into master will not change line coverage
✔️ Merging #3379 into master will will increase branch coverage by 1%
⛔ Merging #3379 into master will will increase complexity by 1

	master	#3379	Change
Lines	`19487` / `27108`	`19581` / `27111`
Lines %	`72%`	`72%`	`0%` ✔️
Branches	`11515` / `16784`	`11603` / `16784`
Branches %	`69%`	`69%`	`1%` ✔️
Complexity	`18173`	`18174`	`1` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3379	Change
Lines %	`72%`	`72%`	`0%` ✔️
Branches %	`69%`	`69%`	`1%` ✔️
Complexity	`18173`	`18174`	`1` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.GitInfo	`-19%` ⛔	`-12%` ⛔	`0` ✔️
Datadog.Trace.Logging.DatadogLogging	`-6%` ⛔	`0%` ✔️	`1` ⛔
Datadog.Trace.TracerManagerFactory	`3%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.TracerManager	`7%` ✔️	`11%` ✔️	`0` ✔️
Datadog.Trace.Telemetry.NullTelemetryController	`9%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.TraceContext	`10%` ✔️	`4%` ✔️	`0` ✔️
Datadog.Trace.Agent.StatsAggregator	`11%` ✔️	`23%` ✔️	`0` ✔️
Datadog.Trace.Processors.ObfuscatorTraceProcessor	`12%` ✔️	`15%` ✔️	`0` ✔️
Datadog.Trace.Processors.NormalizerTraceProcessor	`22%` ✔️	`19%` ✔️	`0` ✔️
Datadog.Trace.Logging.DirectSubmission.Sink.NullDatadogSink	`25%` ✔️	`0%` ✔️	`0` ✔️
...And 8 more

View the full reports for further details:

This comment has been minimized.

Sign in to view

robertpi changed the title ~~only colelct ip for appsec~~ [ASM] only collect ip for appsec Oct 21, 2022

This comment has been minimized.

Sign in to view

github-actions Bot added the area:asm label Oct 21, 2022

This comment has been minimized.

Sign in to view

robertpi and others added 6 commits October 24, 2022 12:21

only colelct ip for appsec

7ca3bd6

update verify files

bc5e4be

Optional tags for ip now

c3dee85

Dont log warning if multiple ip headers found

bfd929a

fix snapshots

38bcf22

regenerate span metadata

2ed03d3

anna-git force-pushed the robert/asm/revert-ip-change branch from e90f95d to 2ed03d3 Compare October 24, 2022 10:22

anna-git marked this pull request as ready for review October 24, 2022 10:22

anna-git requested review from a team as code owners October 24, 2022 10:22

This comment has been minimized.

Sign in to view

andrewlock approved these changes Oct 24, 2022

View reviewed changes

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

This comment has been minimized.

Sign in to view

pierotibou approved these changes Oct 24, 2022

View reviewed changes

fix snapshots

ae2eae6

This comment has been minimized.

Sign in to view

readd edge tags

23bf9a7

anna-git merged commit 0f7c26d into master Oct 24, 2022

anna-git deleted the robert/asm/revert-ip-change branch October 24, 2022 15:11

github-actions Bot added this to the vNext milestone Oct 24, 2022

pierotibou mentioned this pull request Nov 4, 2022

[Tracer] Reintroduce opt-in client IP collection #3419

Merged

Uh oh!

Conversation

robertpi commented Oct 21, 2022 • edited by anna-git Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions Bot commented Oct 24, 2022

Snapshots difference summary

Uh oh!

andrewlock commented Oct 24, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Faster 🎉 in #3379

Raw results

Fewer allocations 🎉 in #3379

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!

andrewlock commented Oct 24, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robertpi commented Oct 21, 2022 •

edited by anna-git

Loading