Create spans for HTTP retries and redirects by denisivan0v · Pull Request #3072 · open-telemetry/opentelemetry-dotnet

denisivan0v · 2022-03-23T00:36:56Z

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

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.
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)

…tivitySourceTests.netfx.cs Co-authored-by: Michael Maxwell <mike.ian.maxwell@gmail.com>

…xtPropagation.cs Co-authored-by: Michael Maxwell <mike.ian.maxwell@gmail.com>

cijothomas · 2022-07-28T22:46:26Z

src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md

  occurs.
  ([#3407](https://github.com/open-telemetry/opentelemetry-dotnet/issues/3407))

+* Create spans for HTTP retries and redirects according to


could you describe the change here itself for users benefit?
Having to read spec and PR to understand what changed is too complicated for end users.

cijothomas · 2022-07-28T22:46:42Z

src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md

  occurs.
  ([#3407](https://github.com/open-telemetry/opentelemetry-dotnet/issues/3407))

+* Create spans for HTTP retries and redirects according to


could you describe the change here itself for users benefit?
Having to read spec and PR to understand what changed is too complicated for end users.

cijothomas · 2022-07-28T22:47:04Z

src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md


+* Create spans for HTTP retries and redirects according to
+  [opentelemetry-specification #2078](https://github.com/open-telemetry/opentelemetry-specification/pull/2078).
+  ([#3072](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3072))


also be specific that this is only done to the HttpClient .NET Core one, and .NET Framework.

github-actions · 2022-08-06T03:32:37Z

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 · 2022-08-13T03:33:06Z

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

vishweshbankwar · 2022-09-26T18:15:42Z

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

vishweshbankwar · 2022-09-26T18:37:19Z

@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 · 2022-10-05T03:44:42Z

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 · 2022-10-10T18:24:12Z

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

cijothomas · 2022-10-10T18:26:55Z

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 · 2022-10-10T19:13:27Z

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 · 2022-10-10T19:47:24Z

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

prateekprshr-nith · 2023-01-04T08:54:53Z

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 · 2023-01-05T17:01:32Z

@prateekprshr-nith This was addressed in #3732

prateekprshr-nith · 2023-01-06T08:11:57Z

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 · 2023-01-09T05:42:00Z

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 · 2023-02-06T06:40:34Z

@Kielek Is this SDK released and available to consume ?

Kielek · 2023-02-06T06:59:04Z

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."

denisivan0v and others added 30 commits December 5, 2021 22:30

Removed auto-instrumentation suppression if request contains traceparent

17492eb

Typo

06e16de

net461 build fix

a72d148

Added request retry instrumentation

c0081fa

net461 test asserts adjusted

51f9c7d

Merge branch 'main' into denisivan0v/http-retries-redirects

e525d4e

Added HttpWebRequest instrumentation

ee7a37f

Code style fix

ae35eea

Code fixes

5cf93b4

Two tests adjusted to the new behavior

0a6bf0b

Code style fixes

a477f2a

Code and tests fixes

58e92f5

HttpWebRequestInstrumentationWorksIfAlreadyInstrumented test adjusted

b961b78

Test fix

5980e60

More test adjustments

d669628

More test adjustments

1a20acc

Minors

138120e

Test fixes

3cec856

Experiment with AsyncLocal

203df89

AsyncLocal adjustments

770907c

TestRedirectedRequest test adjusted

54859f9

Naming

ce1a7a9

Improved implementation and added tests

a771451

Code style fix

2adc1af

Code style fix

abed0c1

Trigger Build

f7fef66

Update test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestAc…

63358b3

…tivitySourceTests.netfx.cs Co-authored-by: Michael Maxwell <mike.ian.maxwell@gmail.com>

Update src/OpenTelemetry.Instrumentation.Http/HttpRequestMessageConte…

2a693e1

…xtPropagation.cs Co-authored-by: Michael Maxwell <mike.ian.maxwell@gmail.com>

Merge branch 'main' into denisivan0v/http-retries-redirects

c8e2fd0

Indentation fixed

1145ef7

vishweshbankwar approved these changes Jul 26, 2022

View reviewed changes

cijothomas and others added 2 commits July 28, 2022 07:31

Merge branch 'main' into denisivan0v/http-retries-redirects

c9c811a

Addressed review comments

cec578f

cijothomas reviewed Jul 28, 2022

View reviewed changes

Merge branch 'main' into denisivan0v/http-retries-redirects

b3fe033

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 6, 2022

github-actions bot closed this Aug 13, 2022

cijothomas reopened this Sep 26, 2022

github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 27, 2022

This was referenced Sep 27, 2022

[HttpClient] Unit test to validate span creation in case of redirect #3699

Merged

[HttpClient] Export span even if request was instrumented #3700

Closed

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 5, 2022

cijothomas closed this Oct 10, 2022

Conversation

denisivan0v commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Uh oh!

cijothomas Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

cijothomas Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

cijothomas Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 6, 2022

Uh oh!

github-actions bot commented Aug 13, 2022

Uh oh!

vishweshbankwar commented Sep 26, 2022

Uh oh!

vishweshbankwar commented Sep 26, 2022

Uh oh!

github-actions bot commented Oct 5, 2022

Uh oh!

mirekkukla commented Oct 10, 2022

Uh oh!

cijothomas commented Oct 10, 2022

Uh oh!

vishweshbankwar commented Oct 10, 2022

Uh oh!

cijothomas commented Oct 10, 2022

Uh oh!

prateekprshr-nith commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanwest commented Jan 5, 2023

Uh oh!

prateekprshr-nith commented Jan 6, 2023

Uh oh!

Kielek commented Jan 9, 2023

Uh oh!

pvsraviteja commented Feb 6, 2023

Uh oh!

Kielek commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

denisivan0v commented Mar 23, 2022 •

edited

Loading

prateekprshr-nith commented Jan 4, 2023 •

edited

Loading