Explore using GitHub etags to lower API use

[premun]

Context

In order to reduce the amount of calls we make to GitHub, we could use etags.
These requests don't count against the quota and we could use these to determine if a PR changed since the last time we saw it.

Goal

Explore or even implement a way to utilize etags during our PR checks.

Notes

• https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api?apiVersion=2022-11-28#use-conditional-requests-if-appropriate

[adamzip]

@premun
I've looked into this and discussed with @dkurepa , we decided that the best option is to create a new interface for remote that will be used in the PCS namespace, and will include redis caching and use of etags. We can call it PcsRemote. PcsRemote will only have methods for objects that can be cached: PR, PR comments, and PR checks.
The code in the darc namespace will still use the original Remote as it is, without providing etags. In fact, maybe darc never even uses PR / PR comments / PR checks anyway.

Looking into this has also revealed some other issues with the code. There are a bunch of design smells, e.g:

• The remote class and the classes it depends on could probably all be singletons
• The implementation of remote takes a remote factory as parameter. This is definitely a code smell.
• GitClient is not a singleton, meaning we create and dispose of HttpClients every time we make a call to remote in the PCS. This is not how HttpClient is intended to be used and can lead to a lot of issues.

These patterns might make sense for the darc CLI, but they don't make sense in a long-running server. They were probably ported without being redesigned for the PCS.

[dkurepa]

This is not how HttpClient is intended to be used and can lead to a lot of issues.

This is not entirely true, you can either have a long living HttpClient (a singleton), or you can use IHttpClientFactory to create "single use" ones, and then dispose of them. https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines#:~:text=To%20summarize%20recommended%20HttpClient%20use%20in%20terms%20of%20lifetime%20management%2C%20you%20should%20use%20either%20long%2Dlived%20clients%20and%20set%20PooledConnectionLifetime%20(.NET%20Core%20and%20.NET%205%2B)%20or%20short%2Dlived%20clients%20created%20by%20IHttpClientFactory.

[adamzip]

Opened an issue for implementing e-tags: #4842

[premun]

@adamzip can you explain some of these more? To me, they sound like quite the opposite of what I think is right so there seems to be some gap (might be on either side).

• Why singleton? That's very scary to be honest - what if it depends on something transient such as token provider (example)? Does it lock in a single instance forever even though that class did not expect to be used this way? Would it then be unable to give you a fresh token? How would this work with scoped classes like Logger or telemetry which contain data scoped to a given request (e.g. the operationId)? In my view, there's a VERY little amount of cases where a class actually needs to be a singleton.
• Why is having a factory as a parameter a smell? Can you elaborate? What if the remote needs to update dependencies in a remote repo and needs to query another repo (like Arcade)? Then it needs to create a remote to that other repo to load necessary information.
• Which GitClient? It's possible it's implemented wrong but the general idea is you register HTTP clients with AddHttpClient() and then you inject HttpClient or use the factory. The DI / factory then (not-)creates these as it sees fit. If it's not so and we create clients manually, it's probably wrong. Can you point me to the place?