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

Starting work on HttpClientFactory documentation #5483

Merged
merged 44 commits into from
May 2, 2018

Conversation

stevejgordon
Copy link
Contributor

@stevejgordon stevejgordon commented Feb 15, 2018

Fixes #5402

This is an early WIP PR to share the initial hour of work on this piece of documentation. A majority of it is brought across from the HttpClientFactory Wiki by @glennc and @rynowak

I have not proof read this / finished working on the actual wording yet. It's here purely to get early feedback that I'm heading in the right direction at this stage and structuring things correctly.

Thoughts very much welcomed from @scottaddie and the product team.

Internal Review Page

@stevejgordon
Copy link
Contributor Author

I went with the working title of Making HTTP requests, welcome thoughts on that in terms of discoverability and accuracy as strictly it's not covering much about actually making requests, just accessing a HttpClient to do so!

@stevejgordon
Copy link
Contributor Author

I need to fix up the casing for some of the headings too!

Copy link
Member

@scottaddie scottaddie left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on! We really appreciate it. See my suggestions.

@@ -136,6 +136,7 @@
## [Microsoft.AspNetCore.All metapackage](xref:fundamentals/metapackage)
## [Choose between .NET Core and .NET Framework](/dotnet/articles/standard/choosing-core-framework-server)
## [Choose between ASP.NET Core and ASP.NET](choose-aspnet-framework.md)
## [Making HTTP Requests](fundamentals/http-requests.md)
Copy link
Member

Choose a reason for hiding this comment

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

  • For titles, we use sentence casing and avoid using gerunds. I suggest a title like "Initiate HTTP requests".
  • For links to other docs, prefer the xref style of linking. In this case, use the following inside the parens:
    xref:fundamentals/http-requests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since changing this it seems to throw a warning on the build system?

@@ -0,0 +1,241 @@
---
title: Making HTTP Requests
Copy link
Member

Choose a reason for hiding this comment

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

For titles, we use sentence casing and avoid using gerunds. I suggest a title like "Initiate HTTP requests".

ms.topic: article
uid: fundamentals/http-requests
---
# Making HTTP Requests
Copy link
Member

Choose a reason for hiding this comment

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

For titles, we use sentence casing and avoid using gerunds. I suggest a title like "Initiate HTTP requests".

---
title: Making HTTP Requests
author: stevejgordon
description: Learn about using the HttpClientFactory features to
Copy link
Member

Choose a reason for hiding this comment

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

Need to complete this sentence.


A `HttpClientFactory` can be registered and used to configure and consume `HttpClient` instances in your application. It provides several benefits:

1. Provides a central location for naming and configuring logical HttpClients. For example, you may configure a “github” client that is pre-configured to access github and a default client for other purposes.
Copy link
Member

Choose a reason for hiding this comment

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

  • Use a bulleted list here instead of a numbered list.
  • github --> GitHub

Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace the smart quotes on "github" with normal straight quotes.

}
```

In this example we only moved configuration into the type, but we could also have methods with behaviour and not actually expose the HttpClient if we want all access to go through this type.
Copy link
Member

Choose a reason for hiding this comment

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

Need a comma after "In this example".

}
```

If we want to entirely encapsulate the HttpClient in our typed client, rather than exposing it as a property we can define our own methods which control the client
Copy link
Member

Choose a reason for hiding this comment

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

  • Need a comma after "rather than exposing it as a property".
  • Need a period at the end of this sentence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be rewritten without the first person.

}
```

In this example the HttpClient is stored as a private field and all access to make external calls goes through the GetValues method.
Copy link
Member

Choose a reason for hiding this comment

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

Need a comma after "In this example".


## Making HTTP requests

For information about using `HttpClientFactory` to access HttpClient instances to make HTTP requests, see [Making HTTP requests](xref:fundamentals/http-requests).
Copy link
Member

Choose a reason for hiding this comment

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

Update the title in the link text to the suggested title (e.g., "Initiate HTTP requests").


Here `AddHttpClient` has been called twice; once with the name 'github' and once without. The github specific client has some default configuration applied, namely the base address and two headers required to work with the GitHub API.

The configuration function here will get called every time CreateClient is called, as a new instance of HttpClient is created each time.
Copy link
Member

Choose a reason for hiding this comment

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

CreateClient --> CreateClient

  • Whenever you're referencing a member name (e.g., class, method, variable, etc.), it should be code-fenced.


## Generated clients

TODO - Refit?
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thanks!

}
```

Once registered, you can accept a `IHttpClientFactory` in your constructor which can then be used to create a `HttpClient`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to generalize this a bit, make sure people know it is DI magic and can work wherever you can inject services. Not just constructors.

@stevejgordon
Copy link
Contributor Author

You're welcome @scottaddie. It's nice to be able to get involved and help. I've updated per the feedback above. I'm hoping to carve some time to do some of the later sections soon.


## Outgoing request middleware

The `HttpClientFactory` supports registering and chaining `DelegatingHandlers` to easily build an outgoing request middleware pipeline. Each of these handlers is able to perform work before and after the outgoing request, in a very similar pattern to the middleware pipeline in ASP.NET Core. This provides a mechanism to manage cross cutting concerns around the requests an app is making. This includes things such as caching, error handling, serialization and logging.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scottaddie Do you have a preference around the code backticks around pluralised types? Inclusive of the "s" or not?

Copy link
Member

@scottaddie scottaddie Feb 16, 2018

Choose a reason for hiding this comment

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

I try to avoid the plural form in backticks, as it looks awkward. I suggest rewriting to something like the following:
"The HttpClientFactory supports registering and chaining a DelegatingHandler to easily build an outgoing request middleware pipeline."

@@ -136,6 +136,7 @@
## [Microsoft.AspNetCore.All metapackage](xref:fundamentals/metapackage)
## [Choose between .NET Core and .NET Framework](/dotnet/articles/standard/choosing-core-framework-server)
## [Choose between ASP.NET Core and ASP.NET](choose-aspnet-framework.md)
## [Initiate HTTP requests](xref:fundamentals/http-requests.md)
Copy link
Member

Choose a reason for hiding this comment

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

Delete the ".md" extension, and this will build fine.

}
```

Once registered, you can accept a `IHttpClientFactory` whereever services can be injected by the DI framework which can then be used to create a `HttpClient` instance.
Copy link
Member

Choose a reason for hiding this comment

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

  • whereever --> anywhere
  • We try to keep sentences as concise as possible. This improves the machine translation result used for localization of the content. I recommend rewriting this as 2 sentences. Maybe something like the following:
    "Once registered, you can accept a IHttpClientFactory anywhere services can be injected by the DI framework. The IHttpClientFactory can then be used to create a HttpClient instance."

}
```

In the preceding code the gitHubClient will have the `BaseAddress` and `DefaultRequestHeaders` set whereas the defaultClient does not. This provides you the with the ability to have different configurations for different purposes. This may mean different configurations per endpoint/API for example.
Copy link
Member

Choose a reason for hiding this comment

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

  • gitHubClient --> gitHubClient
  • defaultClient --> defaultClient


Here `AddHttpClient` has been called twice, once with the name 'github' and once without. The GitHub-specific client has some default configuration applied, namely the base address and two headers required to work with the GitHub API.

The configuration function here will get called every time ceateClient is called, as a new instance of `HttpClient` is created each time.
Copy link
Member

Choose a reason for hiding this comment

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

  • ceateClient --> CreateClient
  • will get called --> is called

@scottaddie
Copy link
Member

scottaddie commented Feb 16, 2018

@stevejgordon Can you also please create an associated ASP.NET Core 2.x Razor Pages app that targets .NET Core 2.x? Your code snippets can then be imported from the sample app instead of embedding the code in the Markdown. See the following doc for guidance: https://github.com/aspnet/Docs/blob/master/CONTRIBUTING.md#code-snippets.

This also allows readers to get the working code from GitHub. The sample app can be put in the following folder, which you'll have to create: fundamentals/http-requests/samples/.

Once the sample app is created, add the following to the top of the doc:

[View or download sample code](https://github.com/aspnet/Docs/tree/master/aspnetcore/fundamentals/http-requests/samples) ([how to download](xref:tutorials/index#how-to-download-a-sample))

@guardrex
Copy link
Collaborator

When there is only one samp, I've been using the singular folder name, sample. I go plural when there is more than one. Let me know if I should start going 100% plural.

@scottaddie
Copy link
Member

@guardrex I prefer the plural form to make the folder name more future-proof. For example, when we also add a sample app for ASP.NET Core 3.x.

@stevejgordon
Copy link
Contributor Author

@scottaddie I can look to get a sample app together for sure. At this point though the HttpClientFactory packages are not available via Nuget. I could put something together against the current MyGet nightlies? Is that okay at this stage, pending the preview 1 release which should allow the sample to be more stable?

@guardrex
Copy link
Collaborator

@stevejgordon A couple more little things for the next pass:

  • There's still future tense (e.g., "will") to convert to present tense.
  • "You" and "your" aren't banned words, but their use should be avoided if possible.

@stevejgordon
Copy link
Contributor Author

Thanks @guardrex - Sure. I'm going to get eyes on this during my lunch break later today and give it a cleanup pass.

@stevejgordon
Copy link
Contributor Author

@guardrex - I've pushed a couple of cleanup passes which focus on removing "you", "your" and "will". I've also tried to clear up and simplify generally.

@scottaddie / @rynowak - I've started (but not pushed) a sample project. It's a bit flaky when running locally (HTTPS not working for me at the moment). The project is pulling in nightly builds from MyGet.

Are we okay to use GitHub as an endpoint in the examples? If it's going to run locally it needs to target a real endpoint.

I'm not convinced a Razor pages based sample makes the most sense since pulling the data from GitHub and rendering feels a bit contrived. Would an API project be more realistic and suited to this?

I'm also trying to assess how to best have a single sample demo the features. My main question is around the ConfigureServices. I can have all of the various registration methods there, but ideally most snippets want just the ones specific to that section. Just a named or just a typed registration. Any thoughts on the best way to approach that?

@guardrex
Copy link
Collaborator

I'm not convinced a Razor pages based sample makes the most sense since pulling the data from GitHub and rendering feels a bit contrived.

Can you put up a repo of the sample you started?

I can have all of the various registration methods there, but ideally most snippets want just the ones specific to that section.

Normally, we'd put #regions around each line or lines for each snippet (i.e., #region snippet1-#endregion, #region snippet2-#endregion, etc.) ... are you saying that approach won't work?


[!code-csharp[](http-requests/samples/Startup.cs?name=snippet5)]

In the preceding code, the `ValidateHeaderHandler` is registered as a transient service with DI. Once registered, `AddHttpMessageHandler` can be called, passing in the type for the handler.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth a callout here that a delegating handler MUST be registered as a transient.


`IHttpClientFactory` integrates with a popular third-party library called [Polly](https://github.com/App-vNext/Polly). Polly is a comprehensive resilience and transient fault-handling library for .NET. It allows developers to express policies such as Retry, Circuit Breaker, Timeout, Bulkhead Isolation, and Fallback in a fluent and thread-safe manner.

Extension methods are provided to enable the use of Polly policies with configured `HttpClient` instances. The Polly extensions are available in a Nuget package called 'Microsoft.Extensions.Http.Polly'. To use the extensions, a PackageReference should be included in the project.
Copy link
Member

Choose a reason for hiding this comment

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

It's worth calling out that Microsoft.Extensions.Http.Polly is not included by Microsoft.AspNetCore.App metapackage, so we expect most apps to add an explicit package reference.

The default experience users will see is that they have access to IHttpClientFactory but not the Polly stuff.

@stevejgordon
Copy link
Contributor Author

Thanks @rynowak - I've updated with feedback so far.


In the preceding code a `WaitAndRetryAsync` policy is defined. Failed requests are retried up to three times with a delay of 600ms between attempts.

### Conditionally adding policies
Copy link
Member

Choose a reason for hiding this comment

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

Can we title this Dynamically selecting policies

{
return issue; // we only want the first object
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handy! I wasn't aware ReadAsAsync was available. I believe (while more complex) the code I have is slightly more efficient? In this example it's taking only the first item. A bit of a contrived example. With ReadAsAsync I get all items back in the list. A very rough comparison is ~150kb extra allocated between memory snapshots.

I spoke to @glennc and @DamianEdwards at Summit as I was concerned about this complexity and if in fact this was a reasonable approach. At that time it seemed like we wanted to include something similar.

I'm happy to make the change though?

Side query: Am I correct in thinking that https://github.com/aspnet/AspNetWebStack/blob/master/src/System.Net.Http.Formatting/HttpContentExtensions.cs is the source building Microsoft.AspNet.WebApi.Client?

Copy link
Member

Choose a reason for hiding this comment

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

If what you're saying is true then we should choose a different example. The focus of this doc isn't on the minutae of how to implement special case handling of JSON scenarios, so it should use the idiomatic approach - which is ReadAsAsync.

Copy link
Contributor Author

@stevejgordon stevejgordon Apr 30, 2018

Choose a reason for hiding this comment

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

I can adjust the sample to return the full list and switch to ReadAsAsync. Not a problem from my side. I'll get that done hopefully later today / tomorrow.

resultObj = JsonConvert.DeserializeObject<IEnumerable<string>>(await result.Content.ReadAsStringAsync()).ToList();
}

return resultObj ?? Array.Empty<string>();
Copy link
Member

Choose a reason for hiding this comment

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

We should show throwing an exception rather than returning null or empty. Just use https://github.com/rynowak/Samples/blob/dev/MusicStore/src/MusicStoreUI/Services/MusicStoreService.cs#L35

services.AddHttpClient("conditionalpolicy")
// Run some code to select a policy based on the request
.AddPolicyHandler(request => request.Method == HttpMethod.Get ? timeout : longTimeout);
#endregion
Copy link
Member

Choose a reason for hiding this comment

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

It's worth calling out somewhere in the Polly part of the docs that users should treat polly policies as long-lived objects. Some policies only function correctly when they are shared or reused. We've worked to make this the idiomatic thing, but it should be covered in the docs as well.

Copy link

@joelhulen joelhulen Apr 27, 2018

Choose a reason for hiding this comment

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

@rynowak Yes, in general we do recommend defining the policy early on and reusing them. Policies are thread-safe and each call to .Execute or .ExecuteAsync establishes a new, independent underlying policy state. Certain policies, as you say, must be shared for maximum effectiveness, like the CircuitBreaker. I can update the Polly and HttpClientFactory doc to reiterate this fact and point to more detailed documentation on the subject.

Choose a reason for hiding this comment

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

Actually, I believe this section of the document explains it clearly enough, do you agree? Use case: scoping CircuitBreakers and Bulkheads


app.UseMvc();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@reisenberger
Copy link
Contributor

Big thanks to @stevejgordon for the work on the HttpClientFactory-Polly side of this doco! I'll aim to review from Polly perspective and feed back, in the next 24-72 hours.

@scottaddie
Copy link
Member

@rynowak How's this doc looking? Any breaking changes coming in RC or beyond that would invalidate this doc? I'm hoping to get this content published ASAP.

Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

There are still two things here that I want to see cleaned up.

  1. The sample code here should use ReadAsAsync everywhere if it's reading objects. No exceptions. Reading the content as a string and then calling JsonConvert is the hard way and is not the idiom we want to teach.

  2. We should not show examples like:

var response = await client.SendAsync(request);
if (response.IsSuccessStatusCode)
{
    // return the data
}
else
{
 return Array.Empty<GitHubPullRequest>();
}

This is not a robust way to do error handling This kind of failure needs to throw using EnsureSuccessStatusCode() and allow it to be handled at a higher level.

Sorry if these seem like nitpicks, but samples matter, these examples will be written once then then copy-pasted thousands of times.

@stevejgordon
Copy link
Contributor Author

stevejgordon commented May 2, 2018 via email

@stevejgordon
Copy link
Contributor Author

@rynowak / @scottaddie - Updated per feedback above.

@@ -32,13 +33,13 @@ public BasicUsageModel(IHttpClientFactory clientFactory)

if (response.IsSuccessStatusCode)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rynowak Do you think this is a reasonable approach when consuming directly inside a Razor Page?

Copy link
Contributor

@reisenberger reisenberger left a comment

Choose a reason for hiding this comment

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

Huge thanks to @stevejgordon for the wonderfully crisp writing. A few minor typos/nits from me here. The only more substantive thing is that we can be precise what AddTransientHttpErrorPolicy handles.


## Outgoing request middleware

`HttpClient` already has the concept of delegating handlers that can be linked together for outgoing HTTP requests. The `IHttpClientFactory` makes registration of per-named clients more intuitive. It supports registration and chaining of multiple handlers to build an outgoing request middleware pipeline. Each of these handlers is able to perform work before and after the outgoing request. This pattern is similar to the inbound middleware pipeline in ASP.NET Core. The pattern provides a mechanism to manage cross-cutting concerns around HTTP requests, including caching, error handling, serialization, and logging.
Copy link
Contributor

Choose a reason for hiding this comment

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

The IHttpClientFactory makes registration of per-named clients more intuitive.

Is there something missing between 'of' and 'per-named'? ("makes registration of delegating handlers per named-clients more intuitive"?)


### Handle transient faults

The most common faults you may expect to occur when making external HTTP calls are transient. A convenient extension method called `AddTransientHttpErrorPolicy` is included, which allows a policy to be defined. The policy applies when these common transient errors occur. Examples of errors handled by the policy include `HttpRequestException`, HTTP 5xx responses, and HTTP 408 responses.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Examples of errors ... include" may suggest there are other kinds of error not mentioned which are also included, but what is stated is the full set. Maybe avoid "includes"?

Could:

Policies configured with this extension method handle HttpRequestException, HTTP 5xx responses, and HTTP 408 responses.


### Handle transient faults

The most common faults you may expect to occur when making external HTTP calls are transient. A convenient extension method called `AddTransientHttpErrorPolicy` is included, which allows a policy to be defined. The policy applies when these common transient errors occur. Examples of errors handled by the policy include `HttpRequestException`, HTTP 5xx responses, and HTTP 408 responses.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could:

A convenient extension method [...] is included, which allows a policy to be defined to handle transient errors.

(and lose following sentence)


## HttpClient and lifetime management

Each time `CreateClient` is called on the `IHttpClientFactory`, a new instance of a `HttpClient` is returned. There's a `HttpMessageHandler` per named client. `IHttpClientFactory` pools the `HttpMessageHandler` instances created by the factory to reduce resource consumption. A `HttpMessageHandler` instance may be reused from the pool when creating new `HttpClient` instance if its lifetime hasn't expired.
Copy link
Contributor

Choose a reason for hiding this comment

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

when creating new HttpClient instance

when creating a new HttpClient instance


The log category used for each client includes the name of the client. A client named "MyNamedClient", for example, logs messages with a category of `System.Net.Http.HttpClient.MyNamedClient.LogicalHandler`. Messages with the suffix of "LogicalHandler" occur on the outside of request handler pipeline. On the request, messages are logged before any other handlers in the pipeline have processed it. On the response, messages are logged after any other pipeline handlers have received the response.

Logging also occurs on the inside of the request handler pipeline. In the "MyNameClient" example, those messages are logged against the log category `System.Net.Http.HttpClient.MyNamedClient.ClientHandler`. For the request, logging occurs after all other handlers have run and immediately before the request is sent out on the network. On the response, this logging includes the state of the response before it passes back through the handler pipeline.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the "MyNameClient" example

"MyNamedClient"

@stevejgordon
Copy link
Contributor Author

Thanks @reisenberger. Good catches. I've fixed up mostly as suggested!

@scottaddie scottaddie changed the title [WIP] Starting work on HttpClientFactory documentation Starting work on HttpClientFactory documentation May 2, 2018
@scottaddie scottaddie merged commit 039252b into dotnet:master May 2, 2018
@scottaddie
Copy link
Member

@stevejgordon Thank you so much for your patience and hard work on this PR. It's always a great feeling to get such high-quality contributions from community members.

@stevejgordon
Copy link
Contributor Author

You're very welcome. It was a pleasure working with you and the team on this. Thanks for your patience in between commits! Hopefully I can find something else to help with in the future.

@stevejgordon stevejgordon deleted the stevejgordon/httpclientfactory branch May 18, 2018 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants