Skip to content

Minor remote config fixes#3347

Merged
robertpi merged 3 commits into
masterfrom
robert/asm/minor-rc-fixes
Oct 20, 2022
Merged

Minor remote config fixes#3347
robertpi merged 3 commits into
masterfrom
robert/asm/minor-rc-fixes

Conversation

@robertpi

@robertpi robertpi commented Oct 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Summary of changes

  • The tracer version must use sematic version numbers: only three point version numbers
  • The field backed_client_state was missing

Test coverage

Add the test to the feature toggle test, where we tests a lot of other RC features.

These should maybe refactored into unit tests in the future?

@robertpi
robertpi requested review from a team as code owners October 12, 2022 16:24
@andrewlock

This comment has been minimized.

Sign in to view
andrewlock
andrewlock approved these changes Oct 13, 2022
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

RE your comment about unit-tests, I agree. It feels like there should be an "example JSON" dumped somewhere, which we can confirm is parsed correctly. I think that would have caught the missing [JsonProperty] for example, and may reduce the need for some of the simpler parts of the integration tests

Comment thread tracer/build/_build/PrepareRelease/SetAllVersions.cs Outdated
Comment thread tracer/src/Datadog.Trace/RemoteConfigurationManagement/RemoteConfigurationManager.cs Outdated
Comment thread tracer/src/Datadog.Trace/TracerConstants.cs Outdated
zacharycmontoya
zacharycmontoya reviewed Oct 14, 2022
View reviewed changes
Comment thread tracer/build/_build/PrepareRelease/SetAllVersions.cs Outdated
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@robertpi
robertpi force-pushed the robert/asm/minor-rc-fixes branch from cb30a93 to d44881c Compare October 18, 2022 10:53
@andrewlock

andrewlock commented Oct 18, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3347 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 Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 825μs 467ns 1.75μs 0.408 0 0 3.22 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 572μs 146ns 566ns 0 0 0 2.63 KB
#3347 WriteAndFlushEnrichedTraces net472 816μs 624ns 2.42μs 0.403 0 0 3.22 KB
#3347 WriteAndFlushEnrichedTraces netcoreapp3.1 534μs 346ns 1.34μs 0 0 0 2.63 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 196ns 0.202ns 0.727ns 0.0701 0 0 441 B
master AllCycleSimpleBody netcoreapp3.1 254ns 0.265ns 1.03ns 0.00587 0 0 440 B
master AllCycleMoreComplexBody net472 194ns 0.0749ns 0.27ns 0.0662 0 0 417 B
master AllCycleMoreComplexBody netcoreapp3.1 251ns 0.132ns 0.494ns 0.00568 0 0 416 B
master BodyExtractorSimpleBody net472 298ns 0.355ns 1.33ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 241ns 0.171ns 0.638ns 0.00374 0 0 272 B
master BodyExtractorMoreComplexBody net472 15.7μs 17.2ns 64.3ns 1.21 0.0157 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 13μs 11.5ns 44.4ns 0.09 0 0 6.75 KB
#3347 AllCycleSimpleBody net472 197ns 0.0788ns 0.284ns 0.07 0 0 441 B
#3347 AllCycleSimpleBody netcoreapp3.1 261ns 0.174ns 0.626ns 0.00601 0 0 440 B
#3347 AllCycleMoreComplexBody net472 196ns 0.0778ns 0.301ns 0.0663 0 0 417 B
#3347 AllCycleMoreComplexBody netcoreapp3.1 251ns 0.138ns 0.515ns 0.00567 0 0 416 B
#3347 BodyExtractorSimpleBody net472 299ns 0.917ns 3.55ns 0.0572 0 0 361 B
#3347 BodyExtractorSimpleBody netcoreapp3.1 246ns 0.117ns 0.437ns 0.00372 0 0 272 B
#3347 BodyExtractorMoreComplexBody net472 15.9μs 15ns 58ns 1.21 0.0159 0 7.62 KB
#3347 BodyExtractorMoreComplexBody netcoreapp3.1 13.1μs 4.63ns 16.7ns 0.0916 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 182μs 249ns 966ns 0.181 0 0 20.66 KB
#3347 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3347 SendRequest netcoreapp3.1 185μs 216ns 837ns 0.185 0 0 20.66 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.84μs 1.02ns 3.97ns 0.158 0.000923 0 995 B
master ExecuteNonQuery netcoreapp3.1 1.39μs 0.813ns 3.15ns 0.0133 0 0 984 B
#3347 ExecuteNonQuery net472 1.68μs 0.555ns 2.15ns 0.158 0.000854 0 995 B
#3347 ExecuteNonQuery netcoreapp3.1 1.44μs 0.401ns 1.5ns 0.0129 0 0 984 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.43μs 0.758ns 2.84ns 0.191 0 0 1.2 KB
master CallElasticsearch netcoreapp3.1 1.53μs 0.529ns 2.05ns 0.0153 0 0 1.14 KB
master CallElasticsearchAsync net472 2.68μs 1.43ns 5.55ns 0.212 0 0 1.34 KB
master CallElasticsearchAsync netcoreapp3.1 1.64μs 0.533ns 2ns 0.0172 0 0 1.26 KB
#3347 CallElasticsearch net472 2.52μs 1.23ns 4.76ns 0.19 0 0 1.2 KB
#3347 CallElasticsearch netcoreapp3.1 1.52μs 0.667ns 2.5ns 0.0156 0 0 1.14 KB
#3347 CallElasticsearchAsync net472 2.68μs 1.97ns 7.63ns 0.213 0 0 1.34 KB
#3347 CallElasticsearchAsync netcoreapp3.1 1.57μs 0.664ns 2.57ns 0.0172 0 0 1.26 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.62μs 1.64ns 6.13ns 0.232 0 0 1.47 KB
master ExecuteAsync netcoreapp3.1 1.79μs 1.11ns 4.15ns 0.0187 0 0 1.39 KB
#3347 ExecuteAsync net472 2.59μs 1.09ns 4.07ns 0.233 0 0 1.47 KB
#3347 ExecuteAsync netcoreapp3.1 1.71μs 1.06ns 4.09ns 0.0187 0 0 1.39 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.91μs 2.51ns 9.73ns 0.445 0 0 2.82 KB
master SendAsync netcoreapp3.1 3.71μs 1.31ns 4.9ns 0.0352 0 0 2.65 KB
#3347 SendAsync net472 5.93μs 6.23ns 24.1ns 0.446 0 0 2.82 KB
#3347 SendAsync netcoreapp3.1 3.63μs 2.22ns 7.99ns 0.0363 0 0 2.65 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.2μs 3.37ns 12.6ns 0.297 0 0 1.88 KB
master EnrichedLog netcoreapp3.1 2.57μs 1.24ns 4.81ns 0.0256 0 0 1.91 KB
#3347 EnrichedLog net472 3.24μs 1.32ns 4.59ns 0.298 0 0 1.88 KB
#3347 EnrichedLog netcoreapp3.1 2.6μs 2.15ns 8.33ns 0.0261 0 0 1.91 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 152μs 147ns 570ns 0.677 0.226 0 4.72 KB
master EnrichedLog netcoreapp3.1 120μs 93.3ns 361ns 0.06 0 0 4.55 KB
#3347 EnrichedLog net472 151μs 83.8ns 325ns 0.675 0.225 0 4.72 KB
#3347 EnrichedLog netcoreapp3.1 120μs 204ns 764ns 0 0 0 4.55 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.78μs 4.92ns 18.4ns 0.58 0.00287 0 3.65 KB
master EnrichedLog netcoreapp3.1 4.33μs 1.92ns 7.42ns 0.0545 0 0 3.98 KB
#3347 EnrichedLog net472 5.66μs 2.27ns 8.19ns 0.577 0.00283 0 3.65 KB
#3347 EnrichedLog netcoreapp3.1 4.54μs 1.48ns 5.72ns 0.054 0 0 3.98 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.3μs 2.72ns 10.2ns 0.225 0 0 1.42 KB
master SendReceive netcoreapp3.1 1.77μs 0.612ns 2.37ns 0.0186 0 0 1.37 KB
#3347 SendReceive net472 2.24μs 4ns 15.5ns 0.225 0 0 1.42 KB
#3347 SendReceive netcoreapp3.1 1.79μs 0.569ns 2.05ns 0.0186 0 0 1.37 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.03μs 2.28ns 8.53ns 0.364 0 0 2.3 KB
master EnrichedLog netcoreapp3.1 4.15μs 1.31ns 4.72ns 0.0248 0 0 1.86 KB
#3347 EnrichedLog net472 5.06μs 1.41ns 5.45ns 0.363 0 0 2.3 KB
#3347 EnrichedLog netcoreapp3.1 4.39μs 1.79ns 6.7ns 0.024 0 0 1.86 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.14μs 0.4ns 1.49ns 0.136 0 0 859 B
master StartFinishSpan netcoreapp3.1 935ns 0.34ns 1.27ns 0.0107 0 0 808 B
master StartFinishScope net472 1.35μs 0.996ns 3.73ns 0.149 0 0 939 B
master StartFinishScope netcoreapp3.1 1.07μs 0.46ns 1.78ns 0.0124 0 0 928 B
#3347 StartFinishSpan net472 1.16μs 0.557ns 2.08ns 0.136 0 0 859 B
#3347 StartFinishSpan netcoreapp3.1 917ns 0.834ns 3.12ns 0.0109 0 0 808 B
#3347 StartFinishScope net472 1.43μs 0.514ns 1.92ns 0.149 0 0 939 B
#3347 StartFinishScope netcoreapp3.1 1.08μs 0.404ns 1.46ns 0.0125 0 0 928 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.48μs 0.451ns 1.75ns 0.149 0 0 939 B
master RunOnMethodBegin netcoreapp3.1 1.12μs 0.576ns 2.23ns 0.0129 0 0 928 B
#3347 RunOnMethodBegin net472 1.52μs 0.291ns 1.09ns 0.149 0 0 939 B
#3347 RunOnMethodBegin netcoreapp3.1 1.1μs 0.448ns 1.62ns 0.0127 0 0 928 B

@andrewlock

andrewlock commented Oct 18, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

✔️ Merging #3347 into master will not change line coverage
✔️ Merging #3347 into master will not change branch coverage
⛔ Merging #3347 into master will will increase complexity by 2

master #3347 Change
Lines 19602 / 27011 19587 / 27012
Lines % 73% 73% 0% ✔️
Branches 11633 / 16784 11625 / 16786
Branches % 69% 69% 0% ✔️
Complexity 18160 18162 2 ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3347 Change
Lines % 73% 73% 0% ✔️
Branches % 69% 69% 0% ✔️
Complexity 18160 18162 2 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change

View the full reports for further details:

dromanol
dromanol reviewed Oct 20, 2022
View reviewed changes
Comment thread tracer/src/Datadog.Trace/TracerConstants.cs
dromanol
dromanol approved these changes Oct 20, 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

@robertpi
robertpi merged commit c9f9112 into master Oct 20, 2022
@robertpi
robertpi deleted the robert/asm/minor-rc-fixes branch October 20, 2022 08:22
@github-actions github-actions Bot added this to the vNext milestone Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes
@andrewlock andrewlock andrewlock approved these changes
@NachoEchevarria NachoEchevarria NachoEchevarria approved these changes
@dromanol dromanol dromanol approved these changes

Assignees

No one assigned

Labels

area:remote-config

Projects

None yet

Milestone

2.18.0

Development

Successfully merging this pull request may close these issues.

6 participants

@robertpi @andrewlock @zacharycmontoya @NachoEchevarria @dromanol @anna-git