Update `http.route` data to align with OpenTelemetry.Instrumentation.AspNetCore output #2989

zacharycmontoya · 2022-07-19T03:16:49Z

Summary of changes

Modifies the addition of the http.route tag (introduced in #2915) to match the value provided by the OpenTelemetry.Instrumentation.AspNetCore package. You can see the current source code in that package here and I've validated by hand that the http.route tags provided by the OTEL instrumentation and the new values embedded in our snapshots match. Snapshot changes include:

The http.route tag is added to all of the NoFF snapshots
For the WithFF snapshots, the http.route tag is removed from the GET / request snapshots because the HttpGetAttribute that decorates its controller action does not have a template
The http.route in the GET /status-code/statusCode request snapshots are updated from status-code-string/{statuscode} to status-code-string/{statusCode} (notice the uppercase C)

To review the snapshot changes, I advise viewing this one commit at a time. Each commit handles both the NoFF/WithFF scenarios for a given test application and its deployment (e.g. AspNetCoreIisMvc30Tests.InProcess, AspNetCoreIisMvc30Tests.OutOfProcess).

Reason for change

Since we are introducing a new tag http.route that is also standardized by OpenTelemetry, I'd like to align the output with the .NET OTEL SDK's instrumentation now so we don't have to update it (and perhaps customer monitors) later.

Implementation details

Updates the AspNetCoreDiagnosticObserver to set the http.route when handling the Microsoft.AspNetCore.Mvc.BeforeAction diagnostic event and removes all other locations where http.route was being set.

Test coverage

Same tests as before, just updated snapshots

Other details

This is targeting the zach/remove-headertags-owintest branch, so please wait until #2986 is merged before merging this.

…ip, http.route, http.useragent Collecting tags, http.url with query string and not raw, http.client_ip, http.route, http.useragent

Fix snapshots fix snapshots fix test name Fix name snapshots

fix public api and fix attempts for mock agent try fix agent port in tests fix snapshots fix snapshots 2.1 fix error contains fix snapshots since rebase and verify helper has the scrubs guids

throughput

Fix return method Fix unit test query string unit tests fix timeout

longer timeout for security tests remove key duplicate in integration test log timeout add log

change way of starting tasks change the way task is initialized with real timeout change way initializing task catch the right exception catch exception better adjust token and crop query string fix substring

snapshots ignore fields reset some files

answer to comments fix api test fixing snapshots fix snapshots response to comments 2 fix publi api tests

…on in OwinWebApi2Tests

…ith the value of typedArg.ActionDescriptor?.AttributeRouteInfo?.Template when the Microsoft.AspNetCore.Mvc.BeforeAction diagnostic source event fires.

Snapshot tests include: - http.route is added to the NoFF tests (in addition to the WithFF test cases) - http.route is removed from the 'GET /' snapshot because the controller action's HttpGetAttribute does not have a route template - The 'GET /status-code/statusCode' requests have a change in case because http.route is set to the exact same value as its route template, which is camel-cased

…pshots. Snapshot tests include: - http.route is added to the NoFF tests (in addition to the WithFF test cases) - http.route is removed from the 'GET /' snapshot because the controller action's HttpGetAttribute does not have a route template - The 'GET /status-code/statusCode' requests have a change in case because http.route is set to the exact same value as its route template, which is camel-cased

…ots. Snapshot tests include: - http.route is added to the NoFF tests (in addition to the WithFF test cases) - http.route is removed from the 'GET /' snapshot because the controller action's HttpGetAttribute does not have a route template - The 'GET /status-code/statusCode' requests have a change in case because http.route is set to the exact same value as its route template, which is camel-cased

Snapshot tests include: - http.route is added to the NoFF tests (in addition to the WithFF test cases) - http.route is removed from the 'GET /' snapshot because the controller action's HttpGetAttribute does not have a route template - The 'GET /status-code/statusCode' requests have a change in case because http.route is set to the exact same value as its route template, which is camel-cased

…allsites where http.route is set, leaving the only setter during the Microsoft.AspNetCore.Mvc.BeforeAction diagnostic source event fires.

Snapshot tests include: - http.route is removed from the 'GET /' snapshot because the controller action's HttpGetAttribute does not have a route template

andrewlock · 2022-07-19T03:58:22Z

Benchmarks Report 🐌

Benchmarks for #2989 compared to master:

1 benchmarks are faster, with geometric mean 1.139
1 benchmarks are slower, with geometric mean 1.123
1 benchmarks have more 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	730μs	635ns	2.46μs	0.361	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	473μs	545ns	2.11μs	0	2.58 KB
#2989	`WriteAndFlushEnrichedTraces`	net472	720μs	660ns	2.47μs	0.355	3.18 KB
#2989	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	458μs	98.3ns	355ns	0	2.58 KB

Benchmarks.Trace.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2989

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.AppSecBodyBenchmark.BodyExtractorSimpleBody‑net472	1.123	249.88	280.70

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	1.68μs	1.59ns	6.16ns	0.237	0	1.49 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	1.87μs	3.58ns	13.8ns	0.0186	0	1.37 KB
master	`AllCycleMoreComplexBody`	net472	16.9μs	20ns	72.2ns	1.39	0.0251	8.75 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	14.2μs	14ns	54.2ns	0.106	0	7.85 KB
master	`BodyExtractorSimpleBody`	net472	250ns	0.35ns	1.31ns	0.0574	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	220ns	0.345ns	1.34ns	0.00364	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	15.2μs	14.4ns	55.8ns	1.2	0.0152	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.8μs	21.4ns	80ns	0.0889	0	6.75 KB
#2989	`AllCycleSimpleBody`	net472	1.68μs	1.58ns	5.9ns	0.237	0	1.49 KB
#2989	`AllCycleSimpleBody`	netcoreapp3.1	1.78μs	1.3ns	4.88ns	0.0187	0	1.37 KB
#2989	`AllCycleMoreComplexBody`	net472	17μs	13.5ns	50.6ns	1.39	0.017	8.75 KB
#2989	`AllCycleMoreComplexBody`	netcoreapp3.1	13.8μs	20.3ns	78.7ns	0.103	0	7.85 KB
#2989	`BodyExtractorSimpleBody`	net472	281ns	0.258ns	0.999ns	0.0573	0	361 B
#2989	`BodyExtractorSimpleBody`	netcoreapp3.1	230ns	0.438ns	1.7ns	0.00368	0	272 B
#2989	`BodyExtractorMoreComplexBody`	net472	15.5μs	12.1ns	45.3ns	1.21	0.0149	7.62 KB
#2989	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.2μs	24.8ns	96.2ns	0.0905	0	6.75 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #2989

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.AspNetCoreBenchmark.SendRequest‑netcoreapp3.1	19.98 KB	20.42 KB	440 B	2.20%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	175μs	89.6ns	347ns	0.263	19.98 KB
#2989	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#2989	`SendRequest`	netcoreapp3.1	180μs	145ns	561ns	0.27	20.42 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.55μs	0.469ns	1.69ns	0.126	0.000775	794 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.32μs	0.475ns	1.78ns	0.0112	0	824 B
#2989	`ExecuteNonQuery`	net472	1.59μs	0.819ns	2.95ns	0.126	0.000795	794 B
#2989	`ExecuteNonQuery`	netcoreapp3.1	1.25μs	0.428ns	1.6ns	0.0112	0	824 B

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

Faster 🎉 in #2989

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync‑netcoreapp3.1	1.139	1,603.58	1,407.95

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.09μs	3.97ns	15.4ns	0.159	1 KB
master	`CallElasticsearch`	netcoreapp3.1	1.39μs	1.97ns	7.64ns	0.0132	984 B
master	`CallElasticsearchAsync`	net472	2.36μs	4.26ns	16.5ns	0.18	1.14 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.6μs	1.85ns	7.17ns	0.0152	1.1 KB
#2989	`CallElasticsearch`	net472	2.18μs	3.31ns	12.8ns	0.159	1 KB
#2989	`CallElasticsearch`	netcoreapp3.1	1.4μs	2.14ns	7.99ns	0.0133	984 B
#2989	`CallElasticsearchAsync`	net472	2.29μs	2.08ns	8.04ns	0.18	1.14 KB
#2989	`CallElasticsearchAsync`	netcoreapp3.1	1.41μs	1.58ns	6.11ns	0.0147	1.1 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.4μs	4.68ns	17.5ns	0.2	1.26 KB
master	`ExecuteAsync`	netcoreapp3.1	1.51μs	3.03ns	11.8ns	0.0167	1.22 KB
#2989	`ExecuteAsync`	net472	2.34μs	6.45ns	25ns	0.199	1.26 KB
#2989	`ExecuteAsync`	netcoreapp3.1	1.56μs	2.23ns	8.65ns	0.0164	1.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.13μs	6.71ns	24.2ns	0.4	2.53 KB
master	`SendAsync`	netcoreapp3.1	3.56μs	4.42ns	15.9ns	0.0338	2.44 KB
#2989	`SendAsync`	net472	5.24μs	12.1ns	47ns	0.4	2.53 KB
#2989	`SendAsync`	netcoreapp3.1	3.51μs	8ns	31ns	0.033	2.44 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3μs	3.59ns	13.9ns	0.263	1.66 KB
master	`EnrichedLog`	netcoreapp3.1	2.35μs	2.09ns	8.11ns	0.0235	1.73 KB
#2989	`EnrichedLog`	net472	2.83μs	1.27ns	4.91ns	0.263	1.66 KB
#2989	`EnrichedLog`	netcoreapp3.1	2.31μs	2.09ns	7.83ns	0.0233	1.73 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	147μs	251ns	974ns	0.671	0.224	4.5 KB
master	`EnrichedLog`	netcoreapp3.1	111μs	179ns	669ns	0.0553	0	4.38 KB
#2989	`EnrichedLog`	net472	149μs	204ns	791ns	0.664	0.221	4.5 KB
#2989	`EnrichedLog`	netcoreapp3.1	112μs	183ns	710ns	0.0566	0	4.38 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.12μs	5.45ns	18.9ns	0.544	0.00256	3.43 KB
master	`EnrichedLog`	netcoreapp3.1	4.11μs	6.41ns	24ns	0.051	0	3.8 KB
#2989	`EnrichedLog`	net472	5.27μs	7.97ns	30.9ns	0.546	0.00262	3.43 KB
#2989	`EnrichedLog`	netcoreapp3.1	4.09μs	9.11ns	35.3ns	0.0513	0	3.8 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	1.97μs	1.9ns	7.37ns	0.192	1.21 KB
master	`SendReceive`	netcoreapp3.1	1.68μs	1.47ns	5.49ns	0.0159	1.2 KB
#2989	`SendReceive`	net472	2.1μs	0.857ns	3.21ns	0.192	1.21 KB
#2989	`SendReceive`	netcoreapp3.1	1.7μs	0.637ns	2.47ns	0.0161	1.2 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	4.59μs	5.48ns	21.2ns	0.329	2.08 KB
master	`EnrichedLog`	netcoreapp3.1	4.1μs	5.16ns	20ns	0.0227	1.69 KB
#2989	`EnrichedLog`	net472	4.59μs	4.93ns	19.1ns	0.328	2.08 KB
#2989	`EnrichedLog`	netcoreapp3.1	4.28μs	4.82ns	18ns	0.0214	1.69 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	826ns	0.377ns	1.46ns	0.104	658 B
master	`StartFinishSpan`	netcoreapp3.1	767ns	0.294ns	1.14ns	0.0089	648 B
master	`StartFinishScope`	net472	1.05μs	0.77ns	2.98ns	0.117	738 B
master	`StartFinishScope`	netcoreapp3.1	862ns	0.488ns	1.76ns	0.0104	768 B
#2989	`StartFinishSpan`	net472	806ns	0.674ns	2.61ns	0.104	658 B
#2989	`StartFinishSpan`	netcoreapp3.1	780ns	0.195ns	0.703ns	0.00901	648 B
#2989	`StartFinishScope`	net472	1.06μs	0.809ns	3.13ns	0.117	738 B
#2989	`StartFinishScope`	netcoreapp3.1	906ns	0.25ns	0.903ns	0.0105	768 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.26μs	0.835ns	3.23ns	0.117	738 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.09μs	0.335ns	1.25ns	0.0103	768 B
#2989	`RunOnMethodBegin`	net472	1.22μs	0.961ns	3.72ns	0.117	738 B
#2989	`RunOnMethodBegin`	netcoreapp3.1	1.06μs	0.38ns	1.47ns	0.0106	768 B

andrewlock

Thanks for the very understandable commit sequence, made it easy to review!🙌 but I have mixed feelings about the PR generally...

I completely agree with you that we should align with OTEL here given we have a clean-slate, and if they don't normalize then we shouldn't either.

But I think there's some issues with the implementation, due to the 2x2 grid of possible routing styles conventional vs attribute and Endpoint vs legacy.

This PR does not include conventional routes. I think that's an error, those endpoints absolutely do have routes. I commented on all the cases of this I saw, just to make sure we catch them all.
I think this will cause an issue for endpoints which don't use MVC. i.e. direct endpoint routed (e.g. minimal APIs) won't trigger the BeforeMvc DianosticObserver event, so I think we'll be missing that tag (I don't see the snapshots updated, so that seems to be the case)
If we've adding the tag to the span 100% of the time, I think this should be a "fast-path" tag property?

andrewlock · 2022-07-21T10:01:41Z

tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreDiagnosticObserver.cs

@@ -793,6 +793,9 @@ private void OnMvcBeforeAction(object arg)
                    {
                        span = StartMvcCoreSpan(tracer, parentSpan, typedArg, httpContext, request);
                    }
+
+                    // Override the http.route to match the OpenTelemetry implementation
+                    parentSpan.SetTag(Tags.HttpRoute, typedArg.ActionDescriptor?.AttributeRouteInfo?.Template);


If we're adding this to every aspnetcore span, we should probably update AspNetCoreTags to use a property instead of falling back to the dictionary slow-path

andrewlock · 2022-07-21T10:03:57Z

...test/snapshots/AspNetCoreIisMvc30Tests.InProcess.WithFF.__path=__statusCode=200.verified.txt

@@ -33,7 +33,6 @@
      http.client_ip: 127.0.0.1,
      http.method: GET,
      http.request.headers.host: localhost:00000,
-      http.route: {controller=home}/{action=index}/{id?},


I don't think this is correct 🤔 Every ASP.NET Core path has a route, it's just that some are conventionally routed and some use attribute routing + some use endpoint routing and some use legacy routing... This is using conventional endpoint routing IIRC....

That's a good point, I've added a comment in this PR to see if OTEL can adopt the conventional routing pattern for their non-attribute routed spans

andrewlock · 2022-07-21T10:04:52Z

...t/snapshots/AspNetCoreIisMvc30Tests.OutOfProcess.WithFF.__path=__statusCode=200.verified.txt

@@ -33,7 +33,6 @@
      http.client_ip: 127.0.0.1,
      http.method: GET,
      http.request.headers.host: localhost:00000,
-      http.route: {controller=home}/{action=index}/{id?},


Same argument as before - conventionally routed requests still have a route

andrewlock · 2022-07-21T10:05:16Z

...t/snapshots/AspNetCoreIisMvc31Tests.OutOfProcess.WithFF.__path=__statusCode=200.verified.txt

@@ -33,7 +33,6 @@
      http.client_ip: 127.0.0.1,
      http.method: GET,
      http.request.headers.host: localhost:00000,
-      http.route: {controller=home}/{action=index}/{id?},


conventional routing

andrewlock · 2022-07-21T10:05:38Z

...test/snapshots/AspNetCoreIisMvc31Tests.InProcess.WithFF.__path=__statusCode=200.verified.txt

@@ -33,7 +33,6 @@
      http.client_ip: 127.0.0.1,
      http.method: GET,
      http.request.headers.host: localhost:00000,
-      http.route: {controller=home}/{action=index}/{id?},


conventional routing

andrewlock · 2022-07-21T10:08:00Z

...napshots/AspNetCoreIisMinimalApisTests.InProcess.WithFF.__path=__statusCode=200.verified.txt

@@ -33,7 +33,6 @@
      http.client_ip: 127.0.0.1,
      http.method: GET,
      http.request.headers.host: localhost:00000,
-      http.route: {controller=home}/{action=index}/{id?},


conventional routing

andrewlock · 2022-07-21T10:08:13Z

tracer/test/snapshots/AspNetCoreMinimalApisTests.WithFF.__path=__statusCode=200.verified.txt

@@ -37,7 +37,6 @@
      http.request.headers.sample_correlation_identifier: 0000-0000-0000,
      http.response.headers.sample_correlation_identifier: 0000-0000-0000,
      http.response.headers.server: Kestrel,
-      http.route: {controller=home}/{action=index}/{id?},


conventional routing

andrewlock · 2022-07-21T10:09:51Z

...pNetCoreIisMinimalApisTests.InProcess.WithFF.__path=_api_delay_0_statusCode=200.verified.txt

@@ -14,7 +14,6 @@
      http.client_ip: 127.0.0.1,
      http.method: GET,
      http.request.headers.host: localhost:00000,
-      http.route: /api/delay/{seconds},


Lies, they do have a route template 😛 It's just a conventional route template

andrewlock · 2022-07-21T10:09:57Z

...tCoreIisMinimalApisTests.OutOfProcess.WithFF.__path=_api_delay_0_statusCode=200.verified.txt

@@ -14,7 +14,6 @@
      http.client_ip: 127.0.0.1,
      http.method: GET,
      http.request.headers.host: localhost:00000,
-      http.route: /api/delay/{seconds},


conventional

andrewlock · 2022-07-21T10:10:01Z

.../snapshots/AspNetCoreMinimalApisTests.WithFF.__path=_api_delay_0_statusCode=200.verified.txt

@@ -18,7 +18,6 @@
      http.request.headers.sample_correlation_identifier: 0000-0000-0000,
      http.response.headers.sample_correlation_identifier: 0000-0000-0000,
      http.response.headers.server: Kestrel,
-      http.route: /api/delay/{seconds},


conventional

anna-git added 30 commits July 15, 2022 16:46

Collecting tags, http.url with query string and not raw, http.client_…

5bda490

…ip, http.route, http.useragent Collecting tags, http.url with query string and not raw, http.client_ip, http.route, http.useragent

factorize ip collection

7d626ee

Collecting and testing route

afe39e9

fix unit tests

adc9753

fix conflicts remove error Id regex, scrubinlineguids should do it

bc6b8d0

integration with aspnet web api 2

85ff054

Ip extraction and tests

de0271a

peer ip in network client ip

bf5edb2

add possibility to disable query string collection

6bec42a

fix url collection and add framework and snapshots

2ce77c4

Fix snapshots fix snapshots fix test name Fix name snapshots

Query string test fix for framework

2312e32

change regex to take into account encoded characters

81622a5

add ability of disabling collecting ip header

6d0aec9

fix public api and fix attempts for mock agent try fix agent port in tests fix snapshots fix snapshots 2.1 fix error contains fix snapshots since rebase and verify helper has the scrubs guids

add throughput tests for query string

5f8ffa8

throughput

add regex pattern to framework and test throuput

8a66358

obfuscator change timeout system for better perf

f8d32e9

Fix return method Fix unit test query string unit tests fix timeout

timeout can be a setting for regex

c30eb0a

add log

07d39ec

Increase timeout

15ca67b

add timeout for integration tests security

02982b3

longer timeout for security tests remove key duplicate in integration test log timeout add log

fix unit tests params

4194b32

logs for instantiation

589d3ce

add timeout logs

60c8b9e

task run instead of factory

ca5b3fa

improve logs

e77de59

change way of starting tasks

7bc267e

change way of starting tasks change the way task is initialized with real timeout change way initializing task catch the right exception catch exception better adjust token and crop query string fix substring

fix tests

25ad075

back to timeout on regex

f99a122

snapshots ignore fields reset some files

fix ignore tags

8931f1d

first modifications to comments

74ca542

anna-git and others added 18 commits July 15, 2022 16:46

add a comment and change variable name to make it clearer

3824c2c

answer to comments

c5c4456

answer to comments fix api test fixing snapshots fix snapshots response to comments 2 fix publi api tests

Remove confusing DD_HEADER_TAGS=User-Agent:http_useragent configurati…

217fdcd

…on in OwinWebApi2Tests

Align AspNetCore http.route logic with OTEL's by setting http.route w…

a6a0720

…ith the value of typedArg.ActionDescriptor?.AttributeRouteInfo?.Template when the Microsoft.AspNetCore.Mvc.BeforeAction diagnostic source event fires.

Align AspNetCore http.route logic with OTEL's by removing all other c…

79fdcf8

…allsites where http.route is set, leaving the only setter during the Microsoft.AspNetCore.Mvc.BeforeAction diagnostic source event fires.

This commit updates the AspNetCore*MinimalApiTests *.WithFF snapshots.

248e2e3

Snapshot tests include: - http.route is removed from the 'GET /' snapshot because the controller action's HttpGetAttribute does not have a route template

zacharycmontoya added the area:tests unit tests, integration tests label Jul 19, 2022

zacharycmontoya requested a review from a team as a code owner July 19, 2022 03:16

zacharycmontoya self-assigned this Jul 19, 2022

andrewlock reviewed Jul 21, 2022

View reviewed changes

zacharycmontoya force-pushed the zach/remove-headertags-owintest branch from 217fdcd to cebed4c Compare July 26, 2022 14:17

zacharycmontoya requested review from a team as code owners July 26, 2022 14:17

Base automatically changed from zach/remove-headertags-owintest to master July 26, 2022 17:14

zacharycmontoya closed this Sep 19, 2022

zacharycmontoya deleted the zach/http-route-otel branch May 9, 2024 23:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `http.route` data to align with OpenTelemetry.Instrumentation.AspNetCore output #2989

Update `http.route` data to align with OpenTelemetry.Instrumentation.AspNetCore output #2989

zacharycmontoya commented Jul 19, 2022

andrewlock commented Jul 19, 2022

Raw results

Slower ⚠️ in #2989

Raw results

More allocations ⚠️ in #2989

Raw results

Raw results

Faster 🎉 in #2989

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

andrewlock left a comment

andrewlock Jul 21, 2022

andrewlock Jul 21, 2022

zacharycmontoya Jul 26, 2022

andrewlock Jul 21, 2022

andrewlock Jul 21, 2022

andrewlock Jul 21, 2022

andrewlock Jul 21, 2022

andrewlock Jul 21, 2022

andrewlock Jul 21, 2022

andrewlock Jul 21, 2022

andrewlock Jul 21, 2022

Update http.route data to align with OpenTelemetry.Instrumentation.AspNetCore output #2989

Update http.route data to align with OpenTelemetry.Instrumentation.AspNetCore output #2989

Conversation

zacharycmontoya commented Jul 19, 2022

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

andrewlock commented Jul 19, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Slower ⚠️ in #2989

Raw results

More allocations ⚠️ in #2989

Raw results

Raw results

Faster 🎉 in #2989

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

andrewlock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update `http.route` data to align with OpenTelemetry.Instrumentation.AspNetCore output #2989

Update `http.route` data to align with OpenTelemetry.Instrumentation.AspNetCore output #2989