Create spans for HTTP retries and redirects

[denisivan0v]

Summary

This PR brings an implementation for HTTP retries and redirects instrumentation defined in opentelemetry-specification #2078. This is the second attempt to add the implementation (#2756 was the first one) with the reduced scope to reflect the current state of HTTP semantic conventions specification.

It addresses a scenario which is in the scope for bringing the existing HTTP semantic conventions for tracing to an initial stable state, see related otep #174.

Changes

1. Now spans will be created for all retry and redirect attempts (previously a span was created for the first attempt only).
   1. A span will be created even though traceparent header is already present.
   2. traceparent header will be overwritten based on current ambient context (Activity.Current) values.
2. We now follow the new spec for retries ad redirects (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-request-retries-and-redirects)

[codecov[bot]]

Codecov Report

Merging #3072 (b3fe033) into main (0a4a625) will decrease coverage by 0.13%. The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3072      +/-   ##
==========================================
- Coverage   86.58%   86.44%   -0.14%     
==========================================
  Files         275      275              
  Lines        9978     9976       -2     
==========================================
- Hits         8639     8624      -15     
- Misses       1339     1352      +13     

Impacted Files	Coverage Δ
...tp/Implementation/HttpHandlerDiagnosticListener.cs	72.50% <ø> (-1.00%)	:arrow_down:
...ation.Http/HttpRequestMessageContextPropagation.cs	35.71% <100.00%> (-64.29%)	:arrow_down:
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	73.52% <0.00%> (-8.83%)	:arrow_down:
...etry.Api/Context/Propagation/PropagationContext.cs	57.14% <0.00%> (-7.15%)	:arrow_down:

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[denisivan0v]

@open-telemetry/dotnet-approvers can you please reopen the PR?

[denisivan0v]

Changes from this PR were benchmarked with the model application submitted here https://github.com/denisivan0v/http-semantic-conventions-experiments/pull/2. The main idea of the setup is to compare the current ("original") implementation (the copy of it is here) vs the modified ("improved") one (the copy of it is here).

Here are results on my machine:

1. Retries are implemented with a naive DelegatingHandler (see SimpleRetryHandler).

BenchmarkDotNet=v0.13.1, OS=macOS Monterey 12.3.1 (21E258) [Darwin 21.4.0] Intel Core i7-1068NG7 CPU 2.30GHz, 1 CPU, 8 logical and 4 physical cores .NET SDK=6.0.101 [Host] : .NET 5.0.13 (5.0.1321.56516), X64 RyuJIT DefaultJob : .NET 5.0.13 (5.0.1321.56516), X64 RyuJIT

Method	Mean	Error	StdDev	Median	Ratio	RatioSD	Gen 0	Gen 1	Allocated
HandlerOriginal	739.3 us	14.46 us	21.65 us	729.2 us	1.00	0.00	16.6016	0.9766	70 KB
HandlerImproved	687.8 us	13.73 us	33.69 us	673.5 us	0.94	0.06	17.5781	-	72 KB

2. Retries are implemented with Polly (see https://github.com/denisivan0v/http-semantic-conventions-experiments/blob/no-links-benchmarks/HttpRetriesApp/Program.cs#L136).

BenchmarkDotNet=v0.13.1, OS=macOS Monterey 12.3.1 (21E258) [Darwin 21.4.0] Intel Core i7-1068NG7 CPU 2.30GHz, 1 CPU, 8 logical and 4 physical cores .NET SDK=6.0.101 [Host] : .NET 5.0.13 (5.0.1321.56516), X64 RyuJIT DefaultJob : .NET 5.0.13 (5.0.1321.56516), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Allocated
PollyOriginal	744.8 us	4.26 us	3.56 us	1.00	18.5547	0.9766	77 KB
PollyImproved	740.4 us	4.48 us	3.97 us	0.99	18.5547	-	77 KB

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[denisivan0v]

The changelog or pr description is not good enough for an end user to understand whats going to happen now onwards. (like we now create span even though traceparent was already present.. we overwrite the traceparent.. we now follow the new spec for redirect/retry.)

I've updated the PR description to make it more clear for an end user how the instrumentation's behavior will change and why it has been done.

[denisivan0v]

Will wait for @CodeBlanch to approve the .NET Framework part, as he has the most expertise of the complexity involved in the working on .NET Framework HttpClient reflection.

@CodeBlanch can you please have another look?

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[denisivan0v]

@open-telemetry/dotnet-maintainers can you please reopen this PR and have another look?

[denisivan0v]

@cijothomas / @CodeBlanch / @vishweshbankwar can you please merge this PR in case it looks good for you?

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[vishweshbankwar]

@cijothomas - Please reopen and merge this PR. I can address the comments in a separate PR. Thanks! @denisivan0v FYI

[vishweshbankwar]

@cijothomas - Please reopen and merge this PR. I can address the comments in a separate PR. Thanks! @denisivan0v FYI

Looks like we have merge conflicts. @denisivan0v - I will create a separate PR using this one and tag you there.

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[mirekkukla]

We're very much looking to this being merged to finally resolve https://github.com/microsoft/ApplicationInsights-dotnet/issues/2556

[cijothomas]

We're very much looking to this being merged to finally resolve microsoft/ApplicationInsights-dotnet#2556

This will not fix anything in ApplicationInsights, unless you are using OpenTelemetry based application insights.

@vishweshbankwar this PR can be now closed as this functionality is already merged now?

[vishweshbankwar]

We're very much looking to this being merged to finally resolve microsoft/ApplicationInsights-dotnet#2556

This will not fix anything in ApplicationInsights, unless you are using OpenTelemetry based application insights.

@vishweshbankwar this PR can be now closed as this functionality is already merged now?

Yes - This can be closed now.

[cijothomas]

Closing as this is covered by alternative PR. Thanks @denisivan0v and @vishweshbankwar

[prateekprshr-nith]

Closing as this is covered by alternative PR. Thanks @denisivan0v and @vishweshbankwar

@cijothomas, can you please provide a link to alternative PR / or clarify if this issue is fixed or not?

[alanwest]

@prateekprshr-nith This was addressed in #3732

[prateekprshr-nith]

Thanks @cijothomas, I see that this was released in https://github.com/open-telemetry/opentelemetry-dotnet/releases/tag/core-1.4.0-beta.2.

Is this available in a stable release as well?

[Kielek]

Not yet, the latest release is 1.4.0-rc.1 for core components/1.0.0-rc9.10 for non-core. ETA for 1.4.0 is end of January.

[pvsraviteja]

@Kielek Is this SDK released and available to consume ?

[Kielek]

There is some delay: https://www.nuget.org/packages/OpenTelemetry/1.4.0-rc.3 was released Feb 2, 2023. From SIG notes: "After it simmers for a bit, we’ll ship 1.4 stable in ~2 weeks."