Moved HTTP request telemetry to HttpClient.SendAsync - #41022

Merged
MihaZupan merged 12 commits into
dotnet:masterfrom
ManickaP:mapichov/40776_activity_id
Aug 31, 2020
Merged

Moved HTTP request telemetry to HttpClient.SendAsync#41022
MihaZupan merged 12 commits into
dotnet:masterfrom
ManickaP:mapichov/40776_activity_id

Conversation

@ManickaP

@ManickaPManickaP commented Aug 19, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Moved HTTP request telemetry to HttpClient.SendAsync
Added events for ResponseContent
Added helper methods events

Contributes to #40776

@ManickaP
ManickaP requested a review from a teamAugust 19, 2020 10:18
@ghost

ghost commented Aug 19, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

stephentoub
stephentoub reviewed Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/Resources/Strings.resx Outdated
ManickaP
ManickaP commented Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs Outdated
stephentoub
stephentoub reviewed Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs Outdated
stephentoub
stephentoub reviewed Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs Outdated
stephentoub
stephentoub reviewed Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs Outdated

@stephentoubstephentoubAug 19, 2020

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.

So now you only get the telemetry if you use HtrpClient, but not if you use HttpMessageInvoker?

@ManickaPManickaPAug 19, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes. Though in the previous state of things, you got the telemetry only with SocketsHandler and not with any custom handler...

However, we can introduce child events to cover HttpMessageInvoker etc. This event pair should be the main, all encompassing one. And in case a user wants to use the invoker directly, we should provide them with examples how to plug in their own telemetry to achieve the same.

I've some thoughts about this and I'm trying to put it all in writing in #40776 (not yet finished though).

@ManickaPManickaP mentioned this pull request Aug 19, 2020
Closed
11 tasks
@ManickaP
ManickaPforce-pushed the mapichov/40776_activity_id branch 2 times, most recently from 150322e to 508b24eCompareAugust 19, 2020 11:30
@ManickaP
ManickaPforce-pushed the mapichov/40776_activity_id branch from 508b24e to ec189d4CompareAugust 24, 2020 12:39
@MihaZupan

MihaZupan commented Aug 26, 2020

Copy link
Copy Markdown
Member

/azp run runtime-libraries outerloop

@azure-pipelines

azure-pipelinesBot commented Aug 26, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan

MihaZupan commented Aug 26, 2020

Copy link
Copy Markdown
Member

/azp run runtime-libraries outerloop

@azure-pipelines

azure-pipelinesBot commented Aug 26, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@karelzkarelz added this to the 6.0.0 milestone Aug 27, 2020
@MihaZupan

MihaZupan commented Aug 27, 2020

Copy link
Copy Markdown
Member

/azp run runtime-libraries outerloop

@azure-pipelines

azure-pipelinesBot commented Aug 27, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP

ManickaP commented Aug 28, 2020

Copy link
Copy Markdown
MemberAuthor

This also fixes: #40896

ManickaP
ManickaP commented Aug 28, 2020
View reviewed changes

@ManickaPManickaP left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Two things in HttpMessageInvoker otherwise LGTM. However, I cannot approve since it's my PR originally. @dotnet/ncl could anyone approve once all is addressed?

And thank you for taking this over and finishing ❤️

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Comment threadsrc/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs Outdated
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs Outdated
alnikola
alnikola approved these changes Aug 28, 2020
View reviewed changes

@alnikolaalnikola 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.

Please, resolve all the Marie's comments.
Otherwise, LGTM

@MihaZupan

MihaZupan commented Aug 28, 2020•
edited
Loading

Copy link
Copy Markdown
Member

I pushed more changes here:

  • Changing the RequestStart signature (versionMajor, Minor from int to byte), adding the version policy field
  • Changing ResponseContentBegin to Start/Stop
  • Renaming RequestAborted to RequestFailed
  • Adding Telemetry tests for non-async Sends

@ManickaP@alnikola Please re-review (should be simple to review on a commit-by-commit basis)

@MihaZupan

MihaZupan commented Aug 28, 2020

Copy link
Copy Markdown
Member

/azp run runtime-libraries outerloop

@azure-pipelines

azure-pipelinesBot commented Aug 28, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

alnikola
alnikola approved these changes Aug 28, 2020
View reviewed changes

@alnikolaalnikola 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

ManickaP
ManickaP commented Aug 28, 2020
View reviewed changes

@ManickaPManickaP left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Single comment, otherwise LGTM.
:shipit:

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@MihaZupan

MihaZupan commented Aug 28, 2020

Copy link
Copy Markdown
Member

/azp run runtime-libraries outerloop

@azure-pipelines

azure-pipelinesBot commented Aug 28, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupanMihaZupan mentioned this pull request Aug 28, 2020
Merged
@ManickaP

ManickaP commented Aug 28, 2020

Copy link
Copy Markdown
MemberAuthor

Runtime CI failure: #41511

@ManickaP

ManickaP commented Aug 28, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Outerloop CI Failures:
3x #40798
5x SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent --> #41530
4x SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http2.PostAsync_Cancel_CancellationTokenPassedToContent(content: StreamContent { Headers = [] }, cancellationTokenSource: CancellationTokenSource { IsCancellationRequested = False, Token = System.Threading.CancellationToken }) --> #41531
1x SocketsHttpHandler_HttpClientHandler_ClientCertificates_Test.Manual_CertificateSentMatchesCertificateReceived_Success(numberOfRequests: 6, reuseClient: False)
1x SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled

All of them are timout related and do not seem to be related to the changes made in this PR.

This was referenced Aug 28, 2020
Closed
Closed
@MihaZupan

MihaZupan commented Aug 29, 2020

Copy link
Copy Markdown
Member

/azp run runtime-libraries outerloop

@azure-pipelines

azure-pipelinesBot commented Aug 29, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
MihaZupan merged commit 05efa9b into dotnet:masterAug 31, 2020
@ManickaP
ManickaP deleted the mapichov/40776_activity_id branch August 31, 2020 09:33
MihaZupan added a commit to MihaZupan/runtime that referenced this pull request Aug 31, 2020
@ManickaP@MihaZupan
Moved HTTP request telemetry to HttpClient.SendAsync (dotnet#41022)
c581053
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
@MihaZupanMihaZupan mentioned this pull request Aug 31, 2020
Merged
MihaZupan added a commit that referenced this pull request Aug 31, 2020
@MihaZupan@ManickaP
[release/5.0] Http telemetry changes backport (#41610)
6b1b038
* System.Net Telemetry style changes (#41527)
* Change HandshakeStop SslProtocols parameter to enum
* Parameterize Http11/Http20 events
* Add dummy request to ensure Http2 settings are received
* Moved HTTP request telemetry to HttpClient.SendAsync (#41022)
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
* Add Request/Response Headers/Content Start/Stop events (#41590)
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@karelzkarelz mentioned this pull request Sep 1, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@MihaZupanMihaZupanMihaZupan left review comments
+1 more reviewer
@alnikolaalnikolaalnikola approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Http

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ManickaP@MihaZupan@stephentoub@alnikola@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Moved HTTP request telemetry to HttpClient.SendAsync - #41022
Merged
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Aug 31, 2020
Merged
Moved HTTP request telemetry to HttpClient.SendAsync#41022
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Conversation
@ManickaP
@ManickaPManickaP
commented
Aug 19, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Moved HTTP request telemetry to HttpClient.SendAsync

Added events for ResponseContent

Added helper methods events
Contributes to #40776
@ManickaP
ManickaP
requested a review
from a teamAugust 19, 2020 10:18
@ghost
ghost
commented
Aug 19, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/Resources/Strings.resx
Outdated

ManickaP
ManickaP
commented
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

@stephentoubstephentoubAug 19, 2020

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.

So now you only get the telemetry if you use HtrpClient, but not if you use HttpMessageInvoker?
@ManickaPManickaPAug 19, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. Though in the previous state of things, you got the telemetry only with SocketsHandler and not with any custom handler...
However, we can introduce child events to cover HttpMessageInvoker etc. This event pair should be the main, all encompassing one. And in case a user wants to use the invoker directly, we should provide them with examples how to plug in their own telemetry to achieve the same.
I've some thoughts about this and I'm trying to put it all in writing in #40776 (not yet finished though).
@ManickaPManickaP
mentioned this pull request
Aug 19, 2020

Closed

11 tasks
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
2 times, most recently
from
150322e to
508b24eCompareAugust 19, 2020 11:30
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
from
508b24e to
ec189d4CompareAugust 24, 2020 12:39
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@karelzkarelz
added this to the 6.0.0 milestone
Aug 27, 2020
@MihaZupan
MihaZupan
commented
Aug 27, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 27, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
This also fixes: #40896
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Two things in HttpMessageInvoker otherwise LGTM. However, I cannot approve since it's my PR originally. @dotnet/ncl could anyone approve once all is addressed?
And thank you for taking this over and finishing ❤️
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Comment threadsrc/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Outdated

alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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.

Please, resolve all the Marie's comments.

Otherwise, LGTM
@MihaZupan
MihaZupan
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

Member
I pushed more changes here:
  • Changing the RequestStart signature (versionMajor, Minor from int to byte), adding the version policy field
  • Changing ResponseContentBegin to Start/Stop
  • Renaming RequestAborted to RequestFailed
  • Adding Telemetry tests for non-async Sends
@ManickaP@alnikola Please re-review (should be simple to review on a commit-by-commit basis)
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Single comment, otherwise LGTM.
:shipit:
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupanMihaZupan
mentioned this pull request
Aug 28, 2020
 Merged

@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
Runtime CI failure: #41511
@ManickaP
ManickaP
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Outerloop CI Failures:

3x #40798

5x SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent --> #41530

4x SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http2.PostAsync_Cancel_CancellationTokenPassedToContent(content: StreamContent { Headers = [] }, cancellationTokenSource: CancellationTokenSource { IsCancellationRequested = False, Token = System.Threading.CancellationToken }) --> #41531

1x SocketsHttpHandler_HttpClientHandler_ClientCertificates_Test.Manual_CertificateSentMatchesCertificateReceived_Success(numberOfRequests: 6, reuseClient: False)

1x SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled
All of them are timout related and do not seem to be related to the changes made in this PR.

This was referenced Aug 28, 2020

Closed


Closed

@MihaZupan
MihaZupan
commented
Aug 29, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 29, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 05efa9b
into
dotnet:masterAug 31, 2020
@ManickaP
ManickaP
deleted the

mapichov/40776_activity_id

branch
August 31, 2020 09:33
MihaZupan added a commit
to MihaZupan/runtime
that referenced
this pull request
Aug 31, 2020
@ManickaP@MihaZupan
Moved HTTP request telemetry to HttpClient.SendAsync (dotnet#41022)
c581053
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
@MihaZupanMihaZupan
mentioned this pull request
Aug 31, 2020
 Merged

MihaZupan added a commit
that referenced
this pull request
Aug 31, 2020
@MihaZupan@ManickaP
[release/5.0] Http telemetry changes backport (#41610)
6b1b038
* System.Net Telemetry style changes (#41527)
* Change HandshakeStop SslProtocols parameter to enum
* Parameterize Http11/Http20 events
* Add dummy request to ensure Http2 settings are received
* Moved HTTP request telemetry to HttpClient.SendAsync (#41022)
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
* Add Request/Response Headers/Content Start/Stop events (#41590)
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@karelzkarelz
mentioned this pull request
Sep 1, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@MihaZupanMihaZupanMihaZupan left review comments

+1 more reviewer

@alnikolaalnikolaalnikola approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Http

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ManickaP@MihaZupan@stephentoub@alnikola@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Moved HTTP request telemetry to HttpClient.SendAsync - #41022
Merged
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Aug 31, 2020
Merged
Moved HTTP request telemetry to HttpClient.SendAsync#41022
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Conversation
@ManickaP
@ManickaPManickaP
commented
Aug 19, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Moved HTTP request telemetry to HttpClient.SendAsync

Added events for ResponseContent

Added helper methods events
Contributes to #40776
@ManickaP
ManickaP
requested a review
from a teamAugust 19, 2020 10:18
@ghost
ghost
commented
Aug 19, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/Resources/Strings.resx
Outdated

ManickaP
ManickaP
commented
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

@stephentoubstephentoubAug 19, 2020

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.

So now you only get the telemetry if you use HtrpClient, but not if you use HttpMessageInvoker?
@ManickaPManickaPAug 19, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. Though in the previous state of things, you got the telemetry only with SocketsHandler and not with any custom handler...
However, we can introduce child events to cover HttpMessageInvoker etc. This event pair should be the main, all encompassing one. And in case a user wants to use the invoker directly, we should provide them with examples how to plug in their own telemetry to achieve the same.
I've some thoughts about this and I'm trying to put it all in writing in #40776 (not yet finished though).
@ManickaPManickaP
mentioned this pull request
Aug 19, 2020

Closed

11 tasks
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
2 times, most recently
from
150322e to
508b24eCompareAugust 19, 2020 11:30
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
from
508b24e to
ec189d4CompareAugust 24, 2020 12:39
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@karelzkarelz
added this to the 6.0.0 milestone
Aug 27, 2020
@MihaZupan
MihaZupan
commented
Aug 27, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 27, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
This also fixes: #40896
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Two things in HttpMessageInvoker otherwise LGTM. However, I cannot approve since it's my PR originally. @dotnet/ncl could anyone approve once all is addressed?
And thank you for taking this over and finishing ❤️
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Comment threadsrc/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Outdated

alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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.

Please, resolve all the Marie's comments.

Otherwise, LGTM
@MihaZupan
MihaZupan
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

Member
I pushed more changes here:
  • Changing the RequestStart signature (versionMajor, Minor from int to byte), adding the version policy field
  • Changing ResponseContentBegin to Start/Stop
  • Renaming RequestAborted to RequestFailed
  • Adding Telemetry tests for non-async Sends
@ManickaP@alnikola Please re-review (should be simple to review on a commit-by-commit basis)
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Single comment, otherwise LGTM.
:shipit:
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupanMihaZupan
mentioned this pull request
Aug 28, 2020
 Merged

@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
Runtime CI failure: #41511
@ManickaP
ManickaP
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Outerloop CI Failures:

3x #40798

5x SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent --> #41530

4x SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http2.PostAsync_Cancel_CancellationTokenPassedToContent(content: StreamContent { Headers = [] }, cancellationTokenSource: CancellationTokenSource { IsCancellationRequested = False, Token = System.Threading.CancellationToken }) --> #41531

1x SocketsHttpHandler_HttpClientHandler_ClientCertificates_Test.Manual_CertificateSentMatchesCertificateReceived_Success(numberOfRequests: 6, reuseClient: False)

1x SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled
All of them are timout related and do not seem to be related to the changes made in this PR.

This was referenced Aug 28, 2020

Closed


Closed

@MihaZupan
MihaZupan
commented
Aug 29, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 29, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 05efa9b
into
dotnet:masterAug 31, 2020
@ManickaP
ManickaP
deleted the

mapichov/40776_activity_id

branch
August 31, 2020 09:33
MihaZupan added a commit
to MihaZupan/runtime
that referenced
this pull request
Aug 31, 2020
@ManickaP@MihaZupan
Moved HTTP request telemetry to HttpClient.SendAsync (dotnet#41022)
c581053
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
@MihaZupanMihaZupan
mentioned this pull request
Aug 31, 2020
 Merged

MihaZupan added a commit
that referenced
this pull request
Aug 31, 2020
@MihaZupan@ManickaP
[release/5.0] Http telemetry changes backport (#41610)
6b1b038
* System.Net Telemetry style changes (#41527)
* Change HandshakeStop SslProtocols parameter to enum
* Parameterize Http11/Http20 events
* Add dummy request to ensure Http2 settings are received
* Moved HTTP request telemetry to HttpClient.SendAsync (#41022)
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
* Add Request/Response Headers/Content Start/Stop events (#41590)
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@karelzkarelz
mentioned this pull request
Sep 1, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@MihaZupanMihaZupanMihaZupan left review comments

+1 more reviewer

@alnikolaalnikolaalnikola approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Http

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ManickaP@MihaZupan@stephentoub@alnikola@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Moved HTTP request telemetry to HttpClient.SendAsync - #41022
Merged
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Aug 31, 2020
Merged
Moved HTTP request telemetry to HttpClient.SendAsync#41022
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Conversation
@ManickaP
@ManickaPManickaP
commented
Aug 19, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Moved HTTP request telemetry to HttpClient.SendAsync

Added events for ResponseContent

Added helper methods events
Contributes to #40776
@ManickaP
ManickaP
requested a review
from a teamAugust 19, 2020 10:18
@ghost
ghost
commented
Aug 19, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/Resources/Strings.resx
Outdated

ManickaP
ManickaP
commented
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

@stephentoubstephentoubAug 19, 2020

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.

So now you only get the telemetry if you use HtrpClient, but not if you use HttpMessageInvoker?
@ManickaPManickaPAug 19, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. Though in the previous state of things, you got the telemetry only with SocketsHandler and not with any custom handler...
However, we can introduce child events to cover HttpMessageInvoker etc. This event pair should be the main, all encompassing one. And in case a user wants to use the invoker directly, we should provide them with examples how to plug in their own telemetry to achieve the same.
I've some thoughts about this and I'm trying to put it all in writing in #40776 (not yet finished though).
@ManickaPManickaP
mentioned this pull request
Aug 19, 2020

Closed

11 tasks
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
2 times, most recently
from
150322e to
508b24eCompareAugust 19, 2020 11:30
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
from
508b24e to
ec189d4CompareAugust 24, 2020 12:39
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@karelzkarelz
added this to the 6.0.0 milestone
Aug 27, 2020
@MihaZupan
MihaZupan
commented
Aug 27, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 27, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
This also fixes: #40896
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Two things in HttpMessageInvoker otherwise LGTM. However, I cannot approve since it's my PR originally. @dotnet/ncl could anyone approve once all is addressed?
And thank you for taking this over and finishing ❤️
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Comment threadsrc/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Outdated

alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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.

Please, resolve all the Marie's comments.

Otherwise, LGTM
@MihaZupan
MihaZupan
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

Member
I pushed more changes here:
  • Changing the RequestStart signature (versionMajor, Minor from int to byte), adding the version policy field
  • Changing ResponseContentBegin to Start/Stop
  • Renaming RequestAborted to RequestFailed
  • Adding Telemetry tests for non-async Sends
@ManickaP@alnikola Please re-review (should be simple to review on a commit-by-commit basis)
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Single comment, otherwise LGTM.
:shipit:
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupanMihaZupan
mentioned this pull request
Aug 28, 2020
 Merged

@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
Runtime CI failure: #41511
@ManickaP
ManickaP
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Outerloop CI Failures:

3x #40798

5x SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent --> #41530

4x SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http2.PostAsync_Cancel_CancellationTokenPassedToContent(content: StreamContent { Headers = [] }, cancellationTokenSource: CancellationTokenSource { IsCancellationRequested = False, Token = System.Threading.CancellationToken }) --> #41531

1x SocketsHttpHandler_HttpClientHandler_ClientCertificates_Test.Manual_CertificateSentMatchesCertificateReceived_Success(numberOfRequests: 6, reuseClient: False)

1x SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled
All of them are timout related and do not seem to be related to the changes made in this PR.

This was referenced Aug 28, 2020

Closed


Closed

@MihaZupan
MihaZupan
commented
Aug 29, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 29, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 05efa9b
into
dotnet:masterAug 31, 2020
@ManickaP
ManickaP
deleted the

mapichov/40776_activity_id

branch
August 31, 2020 09:33
MihaZupan added a commit
to MihaZupan/runtime
that referenced
this pull request
Aug 31, 2020
@ManickaP@MihaZupan
Moved HTTP request telemetry to HttpClient.SendAsync (dotnet#41022)
c581053
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
@MihaZupanMihaZupan
mentioned this pull request
Aug 31, 2020
 Merged

MihaZupan added a commit
that referenced
this pull request
Aug 31, 2020
@MihaZupan@ManickaP
[release/5.0] Http telemetry changes backport (#41610)
6b1b038
* System.Net Telemetry style changes (#41527)
* Change HandshakeStop SslProtocols parameter to enum
* Parameterize Http11/Http20 events
* Add dummy request to ensure Http2 settings are received
* Moved HTTP request telemetry to HttpClient.SendAsync (#41022)
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
* Add Request/Response Headers/Content Start/Stop events (#41590)
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@karelzkarelz
mentioned this pull request
Sep 1, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@MihaZupanMihaZupanMihaZupan left review comments

+1 more reviewer

@alnikolaalnikolaalnikola approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Http

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ManickaP@MihaZupan@stephentoub@alnikola@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Moved HTTP request telemetry to HttpClient.SendAsync - #41022
Merged
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Aug 31, 2020
Merged
Moved HTTP request telemetry to HttpClient.SendAsync#41022
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Conversation
@ManickaP
@ManickaPManickaP
commented
Aug 19, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Moved HTTP request telemetry to HttpClient.SendAsync

Added events for ResponseContent

Added helper methods events
Contributes to #40776
@ManickaP
ManickaP
requested a review
from a teamAugust 19, 2020 10:18
@ghost
ghost
commented
Aug 19, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/Resources/Strings.resx
Outdated

ManickaP
ManickaP
commented
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

@stephentoubstephentoubAug 19, 2020

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.

So now you only get the telemetry if you use HtrpClient, but not if you use HttpMessageInvoker?
@ManickaPManickaPAug 19, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. Though in the previous state of things, you got the telemetry only with SocketsHandler and not with any custom handler...
However, we can introduce child events to cover HttpMessageInvoker etc. This event pair should be the main, all encompassing one. And in case a user wants to use the invoker directly, we should provide them with examples how to plug in their own telemetry to achieve the same.
I've some thoughts about this and I'm trying to put it all in writing in #40776 (not yet finished though).
@ManickaPManickaP
mentioned this pull request
Aug 19, 2020

Closed

11 tasks
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
2 times, most recently
from
150322e to
508b24eCompareAugust 19, 2020 11:30
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
from
508b24e to
ec189d4CompareAugust 24, 2020 12:39
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@karelzkarelz
added this to the 6.0.0 milestone
Aug 27, 2020
@MihaZupan
MihaZupan
commented
Aug 27, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 27, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
This also fixes: #40896
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Two things in HttpMessageInvoker otherwise LGTM. However, I cannot approve since it's my PR originally. @dotnet/ncl could anyone approve once all is addressed?
And thank you for taking this over and finishing ❤️
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Comment threadsrc/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Outdated

alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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.

Please, resolve all the Marie's comments.

Otherwise, LGTM
@MihaZupan
MihaZupan
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

Member
I pushed more changes here:
  • Changing the RequestStart signature (versionMajor, Minor from int to byte), adding the version policy field
  • Changing ResponseContentBegin to Start/Stop
  • Renaming RequestAborted to RequestFailed
  • Adding Telemetry tests for non-async Sends
@ManickaP@alnikola Please re-review (should be simple to review on a commit-by-commit basis)
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Single comment, otherwise LGTM.
:shipit:
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupanMihaZupan
mentioned this pull request
Aug 28, 2020
 Merged

@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
Runtime CI failure: #41511
@ManickaP
ManickaP
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Outerloop CI Failures:

3x #40798

5x SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent --> #41530

4x SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http2.PostAsync_Cancel_CancellationTokenPassedToContent(content: StreamContent { Headers = [] }, cancellationTokenSource: CancellationTokenSource { IsCancellationRequested = False, Token = System.Threading.CancellationToken }) --> #41531

1x SocketsHttpHandler_HttpClientHandler_ClientCertificates_Test.Manual_CertificateSentMatchesCertificateReceived_Success(numberOfRequests: 6, reuseClient: False)

1x SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled
All of them are timout related and do not seem to be related to the changes made in this PR.

This was referenced Aug 28, 2020

Closed


Closed

@MihaZupan
MihaZupan
commented
Aug 29, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 29, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 05efa9b
into
dotnet:masterAug 31, 2020
@ManickaP
ManickaP
deleted the

mapichov/40776_activity_id

branch
August 31, 2020 09:33
MihaZupan added a commit
to MihaZupan/runtime
that referenced
this pull request
Aug 31, 2020
@ManickaP@MihaZupan
Moved HTTP request telemetry to HttpClient.SendAsync (dotnet#41022)
c581053
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
@MihaZupanMihaZupan
mentioned this pull request
Aug 31, 2020
 Merged

MihaZupan added a commit
that referenced
this pull request
Aug 31, 2020
@MihaZupan@ManickaP
[release/5.0] Http telemetry changes backport (#41610)
6b1b038
* System.Net Telemetry style changes (#41527)
* Change HandshakeStop SslProtocols parameter to enum
* Parameterize Http11/Http20 events
* Add dummy request to ensure Http2 settings are received
* Moved HTTP request telemetry to HttpClient.SendAsync (#41022)
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
* Add Request/Response Headers/Content Start/Stop events (#41590)
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@karelzkarelz
mentioned this pull request
Sep 1, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@MihaZupanMihaZupanMihaZupan left review comments

+1 more reviewer

@alnikolaalnikolaalnikola approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Http

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ManickaP@MihaZupan@stephentoub@alnikola@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Moved HTTP request telemetry to HttpClient.SendAsync - #41022
Merged
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Aug 31, 2020
Merged
Moved HTTP request telemetry to HttpClient.SendAsync#41022
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Conversation
@ManickaP
@ManickaPManickaP
commented
Aug 19, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Moved HTTP request telemetry to HttpClient.SendAsync

Added events for ResponseContent

Added helper methods events
Contributes to #40776
@ManickaP
ManickaP
requested a review
from a teamAugust 19, 2020 10:18
@ghost
ghost
commented
Aug 19, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/Resources/Strings.resx
Outdated

ManickaP
ManickaP
commented
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

@stephentoubstephentoubAug 19, 2020

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.

So now you only get the telemetry if you use HtrpClient, but not if you use HttpMessageInvoker?
@ManickaPManickaPAug 19, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. Though in the previous state of things, you got the telemetry only with SocketsHandler and not with any custom handler...
However, we can introduce child events to cover HttpMessageInvoker etc. This event pair should be the main, all encompassing one. And in case a user wants to use the invoker directly, we should provide them with examples how to plug in their own telemetry to achieve the same.
I've some thoughts about this and I'm trying to put it all in writing in #40776 (not yet finished though).
@ManickaPManickaP
mentioned this pull request
Aug 19, 2020

Closed

11 tasks
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
2 times, most recently
from
150322e to
508b24eCompareAugust 19, 2020 11:30
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
from
508b24e to
ec189d4CompareAugust 24, 2020 12:39
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@karelzkarelz
added this to the 6.0.0 milestone
Aug 27, 2020
@MihaZupan
MihaZupan
commented
Aug 27, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 27, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
This also fixes: #40896
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Two things in HttpMessageInvoker otherwise LGTM. However, I cannot approve since it's my PR originally. @dotnet/ncl could anyone approve once all is addressed?
And thank you for taking this over and finishing ❤️
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Comment threadsrc/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Outdated

alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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.

Please, resolve all the Marie's comments.

Otherwise, LGTM
@MihaZupan
MihaZupan
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

Member
I pushed more changes here:
  • Changing the RequestStart signature (versionMajor, Minor from int to byte), adding the version policy field
  • Changing ResponseContentBegin to Start/Stop
  • Renaming RequestAborted to RequestFailed
  • Adding Telemetry tests for non-async Sends
@ManickaP@alnikola Please re-review (should be simple to review on a commit-by-commit basis)
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Single comment, otherwise LGTM.
:shipit:
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupanMihaZupan
mentioned this pull request
Aug 28, 2020
 Merged

@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
Runtime CI failure: #41511
@ManickaP
ManickaP
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Outerloop CI Failures:

3x #40798

5x SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent --> #41530

4x SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http2.PostAsync_Cancel_CancellationTokenPassedToContent(content: StreamContent { Headers = [] }, cancellationTokenSource: CancellationTokenSource { IsCancellationRequested = False, Token = System.Threading.CancellationToken }) --> #41531

1x SocketsHttpHandler_HttpClientHandler_ClientCertificates_Test.Manual_CertificateSentMatchesCertificateReceived_Success(numberOfRequests: 6, reuseClient: False)

1x SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled
All of them are timout related and do not seem to be related to the changes made in this PR.

This was referenced Aug 28, 2020

Closed


Closed

@MihaZupan
MihaZupan
commented
Aug 29, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 29, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 05efa9b
into
dotnet:masterAug 31, 2020
@ManickaP
ManickaP
deleted the

mapichov/40776_activity_id

branch
August 31, 2020 09:33
MihaZupan added a commit
to MihaZupan/runtime
that referenced
this pull request
Aug 31, 2020
@ManickaP@MihaZupan
Moved HTTP request telemetry to HttpClient.SendAsync (dotnet#41022)
c581053
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
@MihaZupanMihaZupan
mentioned this pull request
Aug 31, 2020
 Merged

MihaZupan added a commit
that referenced
this pull request
Aug 31, 2020
@MihaZupan@ManickaP
[release/5.0] Http telemetry changes backport (#41610)
6b1b038
* System.Net Telemetry style changes (#41527)
* Change HandshakeStop SslProtocols parameter to enum
* Parameterize Http11/Http20 events
* Add dummy request to ensure Http2 settings are received
* Moved HTTP request telemetry to HttpClient.SendAsync (#41022)
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
* Add Request/Response Headers/Content Start/Stop events (#41590)
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@karelzkarelz
mentioned this pull request
Sep 1, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@MihaZupanMihaZupanMihaZupan left review comments

+1 more reviewer

@alnikolaalnikolaalnikola approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Http

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ManickaP@MihaZupan@stephentoub@alnikola@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Moved HTTP request telemetry to HttpClient.SendAsync - #41022
Merged
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Aug 31, 2020
Merged
Moved HTTP request telemetry to HttpClient.SendAsync#41022
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Conversation
@ManickaP
@ManickaPManickaP
commented
Aug 19, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Moved HTTP request telemetry to HttpClient.SendAsync

Added events for ResponseContent

Added helper methods events
Contributes to #40776
@ManickaP
ManickaP
requested a review
from a teamAugust 19, 2020 10:18
@ghost
ghost
commented
Aug 19, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/Resources/Strings.resx
Outdated

ManickaP
ManickaP
commented
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

@stephentoubstephentoubAug 19, 2020

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.

So now you only get the telemetry if you use HtrpClient, but not if you use HttpMessageInvoker?
@ManickaPManickaPAug 19, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. Though in the previous state of things, you got the telemetry only with SocketsHandler and not with any custom handler...
However, we can introduce child events to cover HttpMessageInvoker etc. This event pair should be the main, all encompassing one. And in case a user wants to use the invoker directly, we should provide them with examples how to plug in their own telemetry to achieve the same.
I've some thoughts about this and I'm trying to put it all in writing in #40776 (not yet finished though).
@ManickaPManickaP
mentioned this pull request
Aug 19, 2020

Closed

11 tasks
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
2 times, most recently
from
150322e to
508b24eCompareAugust 19, 2020 11:30
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
from
508b24e to
ec189d4CompareAugust 24, 2020 12:39
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@karelzkarelz
added this to the 6.0.0 milestone
Aug 27, 2020
@MihaZupan
MihaZupan
commented
Aug 27, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 27, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
This also fixes: #40896
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Two things in HttpMessageInvoker otherwise LGTM. However, I cannot approve since it's my PR originally. @dotnet/ncl could anyone approve once all is addressed?
And thank you for taking this over and finishing ❤️
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Comment threadsrc/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Outdated

alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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.

Please, resolve all the Marie's comments.

Otherwise, LGTM
@MihaZupan
MihaZupan
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

Member
I pushed more changes here:
  • Changing the RequestStart signature (versionMajor, Minor from int to byte), adding the version policy field
  • Changing ResponseContentBegin to Start/Stop
  • Renaming RequestAborted to RequestFailed
  • Adding Telemetry tests for non-async Sends
@ManickaP@alnikola Please re-review (should be simple to review on a commit-by-commit basis)
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Single comment, otherwise LGTM.
:shipit:
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupanMihaZupan
mentioned this pull request
Aug 28, 2020
 Merged

@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
Runtime CI failure: #41511
@ManickaP
ManickaP
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Outerloop CI Failures:

3x #40798

5x SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent --> #41530

4x SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http2.PostAsync_Cancel_CancellationTokenPassedToContent(content: StreamContent { Headers = [] }, cancellationTokenSource: CancellationTokenSource { IsCancellationRequested = False, Token = System.Threading.CancellationToken }) --> #41531

1x SocketsHttpHandler_HttpClientHandler_ClientCertificates_Test.Manual_CertificateSentMatchesCertificateReceived_Success(numberOfRequests: 6, reuseClient: False)

1x SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled
All of them are timout related and do not seem to be related to the changes made in this PR.

This was referenced Aug 28, 2020

Closed


Closed

@MihaZupan
MihaZupan
commented
Aug 29, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 29, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 05efa9b
into
dotnet:masterAug 31, 2020
@ManickaP
ManickaP
deleted the

mapichov/40776_activity_id

branch
August 31, 2020 09:33
MihaZupan added a commit
to MihaZupan/runtime
that referenced
this pull request
Aug 31, 2020
@ManickaP@MihaZupan
Moved HTTP request telemetry to HttpClient.SendAsync (dotnet#41022)
c581053
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
@MihaZupanMihaZupan
mentioned this pull request
Aug 31, 2020
 Merged

MihaZupan added a commit
that referenced
this pull request
Aug 31, 2020
@MihaZupan@ManickaP
[release/5.0] Http telemetry changes backport (#41610)
6b1b038
* System.Net Telemetry style changes (#41527)
* Change HandshakeStop SslProtocols parameter to enum
* Parameterize Http11/Http20 events
* Add dummy request to ensure Http2 settings are received
* Moved HTTP request telemetry to HttpClient.SendAsync (#41022)
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
* Add Request/Response Headers/Content Start/Stop events (#41590)
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@karelzkarelz
mentioned this pull request
Sep 1, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@MihaZupanMihaZupanMihaZupan left review comments

+1 more reviewer

@alnikolaalnikolaalnikola approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Http

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ManickaP@MihaZupan@stephentoub@alnikola@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Moved HTTP request telemetry to HttpClient.SendAsync - #41022
Merged
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Aug 31, 2020
Merged
Moved HTTP request telemetry to HttpClient.SendAsync#41022
MihaZupan merged 12 commits into
dotnet:masterfrom 
ManickaP:mapichov/40776_activity_id
Conversation
@ManickaP
@ManickaPManickaP
commented
Aug 19, 2020•

edited

Loading

Copy link


Copy Markdown

Member
Moved HTTP request telemetry to HttpClient.SendAsync

Added events for ResponseContent

Added helper methods events
Contributes to #40776
@ManickaP
ManickaP
requested a review
from a teamAugust 19, 2020 10:18
@ghost
ghost
commented
Aug 19, 2020

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl

See info in area-owners.md if you want to be subscribed.
stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/Resources/Strings.resx
Outdated

ManickaP
ManickaP
commented
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

stephentoub
stephentoub
reviewed
Aug 19, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

@stephentoubstephentoubAug 19, 2020

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.

So now you only get the telemetry if you use HtrpClient, but not if you use HttpMessageInvoker?
@ManickaPManickaPAug 19, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. Though in the previous state of things, you got the telemetry only with SocketsHandler and not with any custom handler...
However, we can introduce child events to cover HttpMessageInvoker etc. This event pair should be the main, all encompassing one. And in case a user wants to use the invoker directly, we should provide them with examples how to plug in their own telemetry to achieve the same.
I've some thoughts about this and I'm trying to put it all in writing in #40776 (not yet finished though).
@ManickaPManickaP
mentioned this pull request
Aug 19, 2020

Closed

11 tasks
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
2 times, most recently
from
150322e to
508b24eCompareAugust 19, 2020 11:30
@ManickaP
ManickaPforce-pushed
the
mapichov/40776_activity_id
branch
from
508b24e to
ec189d4CompareAugust 24, 2020 12:39
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
commented
Aug 26, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 26, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@karelzkarelz
added this to the 6.0.0 milestone
Aug 27, 2020
@MihaZupan
MihaZupan
commented
Aug 27, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 27, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
This also fixes: #40896
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Two things in HttpMessageInvoker otherwise LGTM. However, I cannot approve since it's my PR originally. @dotnet/ncl could anyone approve once all is addressed?
And thank you for taking this over and finishing ❤️
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Comment threadsrc/libraries/System.Net.Http/tests/FunctionalTests/TelemetryTest.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
Outdated

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
Outdated

alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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.

Please, resolve all the Marie's comments.

Otherwise, LGTM
@MihaZupan
MihaZupan
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

Member
I pushed more changes here:
  • Changing the RequestStart signature (versionMajor, Minor from int to byte), adding the version policy field
  • Changing ResponseContentBegin to Start/Stop
  • Renaming RequestAborted to RequestFailed
  • Adding Telemetry tests for non-async Sends
@ManickaP@alnikola Please re-review (should be simple to review on a commit-by-commit basis)
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
alnikola
alnikola
approved these changes
Aug 28, 2020
View reviewed changes
@alnikolaalnikola
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
ManickaP
ManickaP
commented
Aug 28, 2020
View reviewed changes
@ManickaPManickaP
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Single comment, otherwise LGTM.
:shipit:
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@MihaZupan
MihaZupan
commented
Aug 28, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupanMihaZupan
mentioned this pull request
Aug 28, 2020
 Merged

@ManickaP
ManickaP
commented
Aug 28, 2020

Copy link


Copy Markdown

MemberAuthor
Runtime CI failure: #41511
@ManickaP
ManickaP
commented
Aug 28, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Outerloop CI Failures:

3x #40798

5x SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent --> #41530

4x SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http2.PostAsync_Cancel_CancellationTokenPassedToContent(content: StreamContent { Headers = [] }, cancellationTokenSource: CancellationTokenSource { IsCancellationRequested = False, Token = System.Threading.CancellationToken }) --> #41531

1x SocketsHttpHandler_HttpClientHandler_ClientCertificates_Test.Manual_CertificateSentMatchesCertificateReceived_Success(numberOfRequests: 6, reuseClient: False)

1x SocketsHttpHandlerTest_Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled
All of them are timout related and do not seem to be related to the changes made in this PR.

This was referenced Aug 28, 2020

Closed


Closed

@MihaZupan
MihaZupan
commented
Aug 29, 2020

Copy link


Copy Markdown

Member
/azp run runtime-libraries outerloop
@azure-pipelines
azure-pipelinesBot
commented
Aug 29, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@MihaZupan
MihaZupan
merged commit 05efa9b
into
dotnet:masterAug 31, 2020
@ManickaP
ManickaP
deleted the

mapichov/40776_activity_id

branch
August 31, 2020 09:33
MihaZupan added a commit
to MihaZupan/runtime
that referenced
this pull request
Aug 31, 2020
@ManickaP@MihaZupan
Moved HTTP request telemetry to HttpClient.SendAsync (dotnet#41022)
c581053
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
@MihaZupanMihaZupan
mentioned this pull request
Aug 31, 2020
 Merged

MihaZupan added a commit
that referenced
this pull request
Aug 31, 2020
@MihaZupan@ManickaP
[release/5.0] Http telemetry changes backport (#41610)
6b1b038
* System.Net Telemetry style changes (#41527)
* Change HandshakeStop SslProtocols parameter to enum
* Parameterize Http11/Http20 events
* Add dummy request to ensure Http2 settings are received
* Moved HTTP request telemetry to HttpClient.SendAsync (#41022)
* Moved HTTP request telemetry to HttpClient.SendAsync
* Added ResponseContent and helper methods events.
* Rework helper method activity nesting
* Expand Telemetry tests
* Also log RequestStart/Stop in HttpMessageInvoker
* Update RequestStart signature
* RequestAborted => RequestFailed rename
* ResponseContent Begin => Start/Stop
* Fix HttpMessageInvoker implementation
* Add Synchronous request Telemetry tests
* Check telemetryStarted before ResponseContentStart
Co-authored-by: MihaZupan <mihazupan.zupan1@gmail.com>
* Add Request/Response Headers/Content Start/Stop events (#41590)
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@karelzkarelz
mentioned this pull request
Sep 1, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@MihaZupanMihaZupanMihaZupan left review comments

+1 more reviewer

@alnikolaalnikolaalnikola approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Http

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ManickaP@MihaZupan@stephentoub@alnikola@karelz