-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Config refactor - fix struct/class T
nullable ref issues, and allow "raw" access to ConfigurationResult
#5715
Config refactor - fix struct/class T
nullable ref issues, and allow "raw" access to ConfigurationResult
#5715
Conversation
f66662c
to
09c39c4
Compare
c21f2bc
to
f1eccb9
Compare
Datadog ReportBranch report: ✅ 0 Failed, 340030 Passed, 1641 Skipped, 13h 50m 30.22s Total Time ⌛ Performance Regressions vs Default Branch (1)
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5715) - mean (75ms) : 66, 84
. : milestone, 75,
master - mean (73ms) : 64, 82
. : milestone, 73,
section CallTarget+Inlining+NGEN
This PR (5715) - mean (1,011ms) : 976, 1047
. : milestone, 1011,
master - mean (989ms) : 970, 1009
. : milestone, 989,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5715) - mean (110ms) : 106, 113
. : milestone, 110,
master - mean (109ms) : 106, 112
. : milestone, 109,
section CallTarget+Inlining+NGEN
This PR (5715) - mean (708ms) : 686, 729
. : milestone, 708,
master - mean (699ms) : 677, 721
. : milestone, 699,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5715) - mean (93ms) : 90, 95
. : milestone, 93,
master - mean (93ms) : 90, 96
. : milestone, 93,
section CallTarget+Inlining+NGEN
This PR (5715) - mean (669ms) : 646, 693
. : milestone, 669,
master - mean (658ms) : 628, 687
. : milestone, 658,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5715) - mean (191ms) : 188, 194
. : milestone, 191,
master - mean (193ms) : 189, 197
. : milestone, 193,
section CallTarget+Inlining+NGEN
This PR (5715) - mean (1,104ms) : 1079, 1129
. : milestone, 1104,
master - mean (1,093ms) : 1068, 1117
. : milestone, 1093,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5715) - mean (278ms) : 272, 283
. : milestone, 278,
master - mean (278ms) : 273, 283
. : milestone, 278,
section CallTarget+Inlining+NGEN
This PR (5715) - mean (880ms) : 860, 900
. : milestone, 880,
master - mean (884ms) : 859, 909
. : milestone, 884,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5715) - mean (266ms) : 262, 269
. : milestone, 266,
master - mean (267ms) : 262, 272
. : milestone, 267,
section CallTarget+Inlining+NGEN
This PR (5715) - mean (866ms) : 842, 889
. : milestone, 866,
master - mean (862ms) : 833, 891
. : milestone, 862,
|
09c39c4
to
d50c73d
Compare
f1eccb9
to
50b3ac7
Compare
Benchmarks Report for appsec 🐌Benchmarks for #5715 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations
|
Benchmark | Base Allocated | Diff Allocated | Change | Change % |
---|---|---|---|---|
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 | 253.71 KB | 255.78 KB | 2.07 KB | 0.82% |
Benchmark | Base Allocated | Diff Allocated | Change | Change % |
---|---|---|---|---|
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 | 59.51 KB | 59.21 KB | -304 B | -0.51% |
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 | 265.94 KB | 255.98 KB | -9.96 KB | -3.75% |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StringConcatBenchmark |
net6.0 | 59.4μs | 599ns | 5.93μs | 0 | 0 | 0 | 43.44 KB |
master | StringConcatBenchmark |
netcoreapp3.1 | 54.4μs | 275ns | 1.23μs | 0 | 0 | 0 | 42.64 KB |
master | StringConcatBenchmark |
net472 | 38.4μs | 107ns | 384ns | 0 | 0 | 0 | 59.51 KB |
master | StringConcatAspectBenchmark |
net6.0 | 307μs | 1.73μs | 11.6μs | 0 | 0 | 0 | 265.94 KB |
master | StringConcatAspectBenchmark |
netcoreapp3.1 | 312μs | 4.51μs | 43.7μs | 0 | 0 | 0 | 253.71 KB |
master | StringConcatAspectBenchmark |
net472 | 280μs | 4.34μs | 42μs | 0 | 0 | 0 | 278.53 KB |
#5715 | StringConcatBenchmark |
net6.0 | 62.2μs | 769ns | 7.65μs | 0 | 0 | 0 | 43.44 KB |
#5715 | StringConcatBenchmark |
netcoreapp3.1 | 63.1μs | 882ns | 8.77μs | 0 | 0 | 0 | 42.64 KB |
#5715 | StringConcatBenchmark |
net472 | 38.1μs | 108ns | 404ns | 0 | 0 | 0 | 59.21 KB |
#5715 | StringConcatAspectBenchmark |
net6.0 | 285μs | 5.61μs | 54.7μs | 0 | 0 | 0 | 255.98 KB |
#5715 | StringConcatAspectBenchmark |
netcoreapp3.1 | 355μs | 1.93μs | 10.6μs | 0 | 0 | 0 | 255.78 KB |
#5715 | StringConcatAspectBenchmark |
net472 | 304μs | 6.99μs | 67.7μs | 0 | 0 | 0 | 278.53 KB |
Benchmarks Report for tracer 🐌Benchmarks for #5715 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Slower
|
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 | 1.152 | 397.67 | 458.10 |
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 | 1.152 | 550.31 | 477.66 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StartFinishSpan |
net6.0 | 397ns | 0.541ns | 2.1ns | 0.00816 | 0 | 0 | 576 B |
master | StartFinishSpan |
netcoreapp3.1 | 641ns | 0.304ns | 1.18ns | 0.00771 | 0 | 0 | 576 B |
master | StartFinishSpan |
net472 | 646ns | 0.364ns | 1.36ns | 0.0918 | 0 | 0 | 578 B |
master | StartFinishScope |
net6.0 | 551ns | 0.448ns | 1.73ns | 0.00968 | 0 | 0 | 696 B |
master | StartFinishScope |
netcoreapp3.1 | 755ns | 0.695ns | 2.41ns | 0.00932 | 0 | 0 | 696 B |
master | StartFinishScope |
net472 | 797ns | 0.682ns | 2.64ns | 0.105 | 0 | 0 | 658 B |
#5715 | StartFinishSpan |
net6.0 | 458ns | 0.209ns | 0.755ns | 0.00809 | 0 | 0 | 576 B |
#5715 | StartFinishSpan |
netcoreapp3.1 | 580ns | 0.374ns | 1.4ns | 0.00787 | 0 | 0 | 576 B |
#5715 | StartFinishSpan |
net472 | 622ns | 0.844ns | 3.27ns | 0.0915 | 0 | 0 | 578 B |
#5715 | StartFinishScope |
net6.0 | 478ns | 0.142ns | 0.551ns | 0.00967 | 0 | 0 | 696 B |
#5715 | StartFinishScope |
netcoreapp3.1 | 697ns | 0.266ns | 0.996ns | 0.00939 | 0 | 0 | 696 B |
#5715 | StartFinishScope |
net472 | 854ns | 0.432ns | 1.67ns | 0.104 | 0 | 0 | 658 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 |
net6.0 | 690ns | 0.199ns | 0.744ns | 0.0097 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 946ns | 0.382ns | 1.48ns | 0.00949 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
net472 | 1.09μs | 0.668ns | 2.59ns | 0.104 | 0 | 0 | 658 B |
#5715 | RunOnMethodBegin |
net6.0 | 676ns | 0.294ns | 1.14ns | 0.00983 | 0 | 0 | 696 B |
#5715 | RunOnMethodBegin |
netcoreapp3.1 | 936ns | 0.262ns | 0.981ns | 0.00935 | 0 | 0 | 696 B |
#5715 | RunOnMethodBegin |
net472 | 1.1μs | 0.465ns | 1.8ns | 0.104 | 0 | 0 | 658 B |
d50c73d
to
a10baba
Compare
50b3ac7
to
6669900
Compare
a10baba
to
85e3f37
Compare
6669900
to
324b11a
Compare
Throughput/Crank Report:zap:Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5715) (11.621M) : 0, 11621419
master (11.593M) : 0, 11593369
benchmarks/2.9.0 (11.542M) : 0, 11542126
section Automatic
This PR (5715) (7.879M) : 0, 7878831
master (7.827M) : 0, 7827212
benchmarks/2.9.0 (8.263M) : 0, 8262905
section Trace stats
master (8.118M) : 0, 8118390
section Manual
This PR (5715) (9.984M) : 0, 9983744
master (9.920M) : 0, 9920204
section Manual + Automatic
This PR (5715) (7.405M) : 0, 7404774
master (7.358M) : 0, 7357979
section Version Conflict
master (6.635M) : 0, 6634804
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5715) (9.499M) : 0, 9498763
master (9.689M) : 0, 9688586
benchmarks/2.9.0 (9.596M) : 0, 9596140
section Automatic
This PR (5715) (6.758M) : 0, 6758328
master (6.298M) : 0, 6298066
section Trace stats
master (6.862M) : 0, 6861645
section Manual
This PR (5715) (8.322M) : 0, 8322285
master (8.028M) : 0, 8028063
section Manual + Automatic
This PR (5715) (6.281M) : 0, 6280596
master (6.139M) : 0, 6139377
section Version Conflict
master (5.667M) : 0, 5667497
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5715) (10.047M) : 0, 10046646
master (10.241M) : 0, 10241408
benchmarks/2.9.0 (10.213M) : 0, 10213239
section Automatic
This PR (5715) (7.088M) : 0, 7087621
master (7.186M) : 0, 7186302
benchmarks/2.9.0 (7.482M) : 0, 7482023
section Trace stats
master (7.568M) : 0, 7568098
section Manual
This PR (5715) (8.847M) : 0, 8847057
master (9.213M) : 0, 9213352
section Manual + Automatic
This PR (5715) (6.863M) : 0, 6863162
master (7.004M) : 0, 7004280
section Version Conflict
master (6.289M) : 0, 6288694
|
85e3f37
to
6ddca1c
Compare
324b11a
to
7b76743
Compare
tracer/src/Datadog.Trace/Configuration/ConfigurationSources/Telemetry/ConfigurationBuilder.cs
Outdated
Show resolved
Hide resolved
// Note: Calling GetAsClass<string>() here instead of GetAsString() as we need to get the | ||
// "serialized JToken", which in JsonConfigurationSource is different, as it allows for non-string tokens | ||
SamplingRules = settings.WithKeys(ConfigurationKeys.CustomSamplingRules).GetAsClass<string>(validator: null, converter: s => s), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 😅
@@ -63,6 +63,55 @@ public ConfigurationBuilder(IConfigurationSource source, IConfigurationTelemetry | |||
|
|||
public HasKeys WithKeys(string key, string fallbackKey1, string fallbackKey2, string fallbackKey3) => new(_source, _telemetry, key, fallbackKey1, fallbackKey2, fallbackKey3); | |||
|
|||
private static bool TryHandleResult<T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It looks like you call this in GetAs<T>
knowing that the getDefaultValue
argument is always non-null, and then in GetAsClass<T> / GetAsStruct<T>
you always pass null. Could you break out TryHandleResult<T>
into two overloads, one where it is present and one where it is not nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you break out TryHandleResult into two overloads, one where it is present and one where it is not nullable?
I definitely could, I was just trying to reduce the amount of duplication 😅 Maybe it's not worth the hassle 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the really annoying thing here is that I can't do that with method overloads. So we're need to call it something like TryHandleResultWithOptionalDefault
or something 😅 and it still doesn't solve the need to use !
because of the weirdness with nullables and struct/classes, so I'm not sure it gains a lot overall 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one small comment!
6ddca1c
to
e26f946
Compare
7b76743
to
248be76
Compare
e26f946
to
28a3186
Compare
248be76
to
b2d6f5a
Compare
…5713) ## Summary of changes - Changes `ITelemetredConfigurationSource` methods to return `ConfigurationResult<T>` instead of `ConfigurationResult<T>?` - Add additional properties to `ConfigurationResult<>` to distinguish between "key not found" and "value not parsable" ## Reason for change In #5661, we added support for mapping standard OTel configuration to DD equivalents. Unfortunately, doing this correctly required distinguishing between "key not found" and "value not parsable". As a workaround, an addition method `IsPresent(key)` was added, but this essentially duplicates a lot of the work we're already doing when loading keys "normally". The changes in this PR are a first step to simplifying and unifying the support for Otel ## Implementation details - Change all `ITelemeteredConfigurationSource.Get*` methods to return `ConfigurationResult` instead of a potentially null value. Update all sources as appropriate. - Update `ConfigurationResult` - Add `bool IsPresent()` - Add `LoadResult` enum which can be one of `Valid`, `NotFound`, `ParsingError`, or `ValidationFailure `. Previously we effectively had a bool of just `Valid` or `ValidationFailure`, and "is null" meant `NotFound` _or_ `ParsingError` - Add helper method `ShouldFallBack` - Refactor `ConfigurationBuilder` - Extract common "cascading fallbacks" code to private `GetResult` methods. By extracting static lambdas we can reuse a lot of the logic for multiple types - Refactor code to use the new properties where possible. - `ITelemeteredConfigurationSource.IsPresent` is still used in Otel paths currently - removed in a subsequent PR ## Test coverage This is just a refactor, so all covered by existing tests ## Other details This is part of a big stack of config refactoring PRs: - #5713 (this PR) - #5714 - #5715 - #5716 - #5717 <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
28a3186
to
811d98d
Compare
b2d6f5a
to
5f72dbc
Compare
…5714) ## Summary of changes Allow providing additional "converters" for types other than the generic `T`, such as `int` and `bool` ## Reason for change The otel-specific code introduced in #5661 requires performing non-standard conversions to some primitive types. For example, for a `bool` the required values might be `"none"`=`true`, which requires using `GetAs<>` to handle the conversion. This PR adds additional overloads so that you can call `GetAsBool` which makes the usages more obvious (and simplifies some things in subsequent PRs) ## Implementation details Add additional, optional, "converter" arguments (`Func<string, ParsingResult<bool>>`) to the methods that don't already have them ## Test coverage Add unit tests for the new conversion behaviour ## Other details This is part of a big stack of config refactoring PRs: - #5713 - #5714 (this PR) - #5715 - #5716 - #5717 <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
…t NRT behaviour Also allow specifying the default value _after_ extracting the result
5f72dbc
to
f14db42
Compare
…figurationBuilder` (#5716) ## Summary of changes - Removes all the `ConfigurationBuilder` method overloads that contained otel-specific code - Refactor the otel-specific code to be in the caller (e.g. `TracerSettings`) not `ConfigurationBuilder` ## Reason for change `ConfigurationBuilder` is used _anywhere_ that we need to load config, which can be very early in the startup process. That limits what we can safely do in there (e.g. logging). Also, having otel-specific functions for remapping keys and changing behaviour in `ConfigurationBuilder` felt wrong. With these changes we still encapsulate the _general_ "override" concept (where a value could come from two very different source key/values), while removing the _specifics_ to the callee. This should allow us to more easily support additional use cases in the future. ## Implementation details - Remove all the "otel-specific" overloads in `ConfigurationBuilder` - Add `OverrideWith()` to `StructConfigurationResultWithKey` and `ClassConfigurationResultWithKey` - Extract the "override" method to be a single, instead of multiply implemented in several places. - Replace all the OTel methods with their equivalent Unfortunately, loading sample rates is still pretty complicated, so the logic for that has _mostly_ just been moved into `TracerSettings` without much simplification. ## Test coverage This is all covered by existing `TracerSettingsTests` and `GlobalSettingsTests`! ## Other details This is part of a big stack of config refactoring PRs: - #5713 - #5714 - #5715 - #5716 (this PR) - #5717 <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
## Summary of changes Records logs and metrics for when otel config is overriden by datadog config, or when it's invalid ## Reason for change This was originally part of #5661, but wasn't possible due to concerns with the fact `ConfigurationBuilder` is used in critical "startup" paths for the tracer, so can result in recursion if we're not careful. ## Implementation details The crux of the implementation is that we "store" error and metrics for writing at the point we know it's safe. This is _similar_ to what we already do for configuration telemetry. (Technically I think we _Could_ directly access the `TelemetryFactory.Metrics`, but this is "safer", and makes it easier to test we're sending the right metrics ## Test coverage Added unit tests to all of the existing settings tests where we override configuration to confirm that it works as expected ## Other details The metrics etc are all ported from #5661, but there are currently some issues with the proposed values: - [x] The metrics aren't added to the intake yet, so need updating there (and copying here) - [x] The tag names aren't "valid" for standard telemetry metrics, they should be lowercase, and shouldn't use `.`. I would suggest changing these to be lowercase and replacing `.` with `_`, but either way this should happen after they've been OK'd in the intake. Part of a big ole stack: - #5713 - #5714 - #5715 - #5716 - #5717 (this PR) <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. --> --------- Co-authored-by: Zach Montoya <zach.montoya@datadoghq.com>
Records logs and metrics for when otel config is overriden by datadog config, or when it's invalid This was originally part of #5661, but wasn't possible due to concerns with the fact `ConfigurationBuilder` is used in critical "startup" paths for the tracer, so can result in recursion if we're not careful. The crux of the implementation is that we "store" error and metrics for writing at the point we know it's safe. This is _similar_ to what we already do for configuration telemetry. (Technically I think we _Could_ directly access the `TelemetryFactory.Metrics`, but this is "safer", and makes it easier to test we're sending the right metrics Added unit tests to all of the existing settings tests where we override configuration to confirm that it works as expected The metrics etc are all ported from #5661, but there are currently some issues with the proposed values: - [x] The metrics aren't added to the intake yet, so need updating there (and copying here) - [x] The tag names aren't "valid" for standard telemetry metrics, they should be lowercase, and shouldn't use `.`. I would suggest changing these to be lowercase and replacing `.` with `_`, but either way this should happen after they've been OK'd in the intake. Part of a big ole stack: - #5713 - #5714 - #5715 - #5716 - #5717 (this PR)
Summary of changes
ConfigurationResult<>
directly from a configuration builder.Reason for change
The first issue is the method
T? GetAs<T>(key)
doesn't behave as we (previously) believed. If you callGetAs<int>(somekey)
we previously were expecting it to returnnull
, but it doesn't - the best you can do is returndefault
i.e. 0.There's no technical way around this limitation AFAIK other than having separate methods, or having methods that are guaranteed non-null returning. So in this PR we have both.
Additionally, there are some cases where it's useful to provide access to the "raw"
ConfigurationResut<T>
. One example is shown in this PR where the security settings want to know both whether the value was set and what the value is, while also providing a default. It will also be useful in subsequent PRs for refactoring the OTel config.Implementation details
Get*Result()
methods onConfigurationBuilder
, e.g.GetBoolResult()
returns aStructConfigurationResultWithKey<bool>
,GetStringResult()
returns aClassConfigurationResultWithKey<bool>
,WithDefault(T default)
on this to get the correct value (and record the default in telemetry correctly)Test coverage
Mostly a refactor, should be covered by existing tests
Other details
This is part of a big stack of config refactoring PRs:
T
nullable ref issues, and allow "raw" access toConfigurationResult
#5715 (this PR)ConfigurationBuilder
#5716