Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying a context propagator on SocketsHttpHandler #55556

Closed
MihaZupan opened this issue Jul 13, 2021 · 3 comments · Fixed by #55392
Closed

Allow specifying a context propagator on SocketsHttpHandler #55556

MihaZupan opened this issue Jul 13, 2021 · 3 comments · Fixed by #55392
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented Jul 13, 2021

Background and Motivation

We approved the Propagator APIs in #50658, which we are consuming in the HttpClient stack.

The approved API includes a global static property Current.

Some advanced cases require more granular control over which propagator is used - for example only enabling propagation for a given handler/destination/library.
Unit testing of applications/libraries is also difficult without resorting to hacks like RemoteExecutor / writing a custom composite propagator / disabling test parallelization.
To address these scenarios, we propose exposing a property on SocketsHttpHandler to control which propagator is used.

Proposed API

namespace System.Net.Http
{
    public sealed class SocketsHttpHandler
    {
        public DistributedContextPropagator? ActivityHeadersPropagator { get; set; }
    }
}

null means no propagation.

Usage Examples

using var handler = new SocketsHttpHandler
{
    ActivityHeadersPropagator = DistributedContextPropagator.CreatePassThroughPropagator()
};

Alternative Designs

Different names for the property, e.g.

  • DistributedContextPropagator? DistributedContextPropagator { get; set; }
    • property name the same as the type name
    • Comment here was that we can be more specific about the property's role specifically for SocketsHttpHandler to hopefully make it easier for users to understand
@MihaZupan MihaZupan added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http labels Jul 13, 2021
@MihaZupan MihaZupan added this to the 6.0.0 milestone Jul 13, 2021
@ghost
Copy link

ghost commented Jul 13, 2021

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

Issue Details

Background and Motivation

We approved the Propagator APIs in #50658, which we are consuming in the HttpClient stack.

The approved API includes a global static property Current.

Some advanced cases require more granular control over which propagator is used - for example only enabling propagation for a given handler/destination/library.
Unit testing of applications/libraries is also difficult without resorting to hacks like RemoteExecutor / writing a custom composite propagator / disabling test parallelization.
To address these scenarios, we propose exposing a property on SocketsHttpHandler to control which propagator is used.

Proposed API

namespace System.Net.Http
{
    public sealed class SocketsHttpHandler
    {
        public DistributedContextPropagator? ActivityHeaderPropagator { get; set; }
    }
}

null means no propagation.

Usage Examples

using var handler = new SocketsHttpHandler
{
    ActivityHeaderPropagator = DistributedContextPropagator.CreatePassThroughPropagator()
};

Alternative Designs

Different names for the property, e.g.

  • DistributedContextPropagator? DistributedContextPropagator { get; set; } - property name same as the type name
Author: MihaZupan
Assignees: -
Labels:

api-suggestion, area-System.Net.Http

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jul 13, 2021
@MihaZupan MihaZupan removed the untriaged New issue has not been triaged by the area owner label Jul 13, 2021
@karelz
Copy link
Member

karelz commented Jul 13, 2021

Update: We changed it to plural -- Headers instead of Header ... discussion with @MihaZupan and @stephentoub

@MihaZupan MihaZupan added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 13, 2021
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 13, 2021
@terrajobst
Copy link
Member

  • Looks good as proposed
namespace System.Net.Http
{
    public sealed class SocketsHttpHandler
    {
        public DistributedContextPropagator? ActivityHeadersPropagator { get; set; }
    }
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants