Revert #1418, #1367, introduce public version of RestMethodInfo

[anaisbetts]

Reverts #1418 and #1367, we will add the functionality of #1367 back as part of a fixed up version of #1317, and we'll re-evaluate #1418. This PR also removes access to the internal type RestMethodInfo, replacing it with a read-only sanitized version. While initially this public type will be limited, we can evaluate adding new fields as-needed.

TODO:

• Revert Adds named httpclient support #1418
• Revert Feature: allow developers to inject the MethodInfo as a Property #1367
• Reimplement Make the RestMethodInfo available in the request options #1317 without using internal types
• add functionality from Feature: allow developers to inject the MethodInfo as a Property #1367 in new public types as needed
• Fix the merge commit that I have inevitably borked
• Docs / tests / etc

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.35 🎉

Comparison is base (ef46395) 89.73% compared to head (c2fe63e) 90.08%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1496      +/-   ##
==========================================
+ Coverage   89.73%   90.08%   +0.35%     
==========================================
  Files          34       33       -1     
  Lines        1705     1685      -20     
==========================================
- Hits         1530     1518      -12     
+ Misses        175      167       -8     

Impacted Files	Coverage Δ
Refit/HttpRequestMessageProperties.cs	100.00% <ø> (ø)
...t.HttpClientFactory/HttpClientFactoryExtensions.cs	84.61% <100.00%> (-0.57%)	⬇️
Refit/RefitSettings.cs	97.05% <100.00%> (-0.13%)	⬇️
Refit/RequestBuilderImplementation.cs	98.00% <100.00%> (-0.03%)	⬇️
Refit/RestMethodInfo.cs	95.60% <100.00%> (+0.03%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[anaisbetts]

@james-s-tayler Can you make sure that the change proposed here still works for your use-case?

[james-s-tayler]

@anaisbetts I think it will.

[cbrevik]

I was looking forward to the changes in #1418 being released, can I ask why it was reverted? Current name schemes coming from the UniqueName-implementation seemingly really bloats up logs / metrics.

[anaisbetts]

@cbrevik I reverted it because the implementation was too Java'y and it was extremely unclear from the PR what problem this actually solves or why it should exist - can you explain further?

[cbrevik]

The problem it solves relates to how other logging and metric libraries logs the name of the HTTP-client.

See for example #1467 in this repository and the issue it links back to from the Serilog-repository: serilog/serilog#1793 (comment)

Serilog pulls the FullName from the HttpClient which normally includes namespace + type, as per the comment: serilog/serilog#1793 (comment)

Logging of "normal" HTTP-clients will be in this format:
2023-01-05 18:45:53.898 +01:00 [INF] System.Net.Http.HttpClient.MyController.LogicalHandler Start processing HTTP request POST http://example.com/stuff

Whilst logging of Refit-clients will be like this:
2023-01-05 18:45:53.861 +01:00 [INF] System.Net.Http.HttpClient.Refit.Implementation.Generated+MyProjectIApi, MyProject, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null.LogicalHandler Start processing HTTP request POST http://example.com/stuff

Which including the prefixed stuff includes Version, PublicKeyToken etc.

Mostly then because of the implementation of https://github.com/reactiveui/refit/blob/main/Refit/UniqueName.cs

I've seen the same issue with for example using Prometheus, a sample metric from there:
httpclient_response_duration_seconds_sum{method="GET",host="api.example.com",client="Refit.Implementation.Generated+MyFancyApiInfrastructureInsightClientIInsightHttpClient, My.Fancy.Api, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null",code="200"} 0.2517555

So the issue is that it really bloats the logs and metrics up, and makes them a bit unreadable. Especially if you have (like we have) several different HTTP-clients with Refit. So I'd like for the generated name to be either shorter by default (and I see from your explanation why it shouldn't), or at least make it overridable if not (like in #1418 or a less Java-y solution)

GitHub

refit/UniqueName.cs at main · reactiveui/refit

The automatic type-safe REST library for .NET Core, Xamarin and .NET. Heavily inspired by Square's Retrofit library, Refit turns your REST API into a live interface. - refit/UniqueName.cs at ma...

[anaisbetts]

@cbrevik Got it, thanks for the explanation. I think we can figure something out then, probably bringing back #1418 but with a bit nicer implementation

[anaisbetts]

@cbrevik Can you confirm that 3248979 works for you?

[cbrevik]

@cbrevik Can you confirm that 3248979 works for you?

It looks like it'll be able to do what I want yeah! I can test it in our project too if a preview nuget-package is released? Unless there is some other easy way for me to test out that commit specifically?

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.