Make BuildUri public

[alexeyzimarev]

From the previous discussion:

The only property not including in the interface is DefaultParameters.
If this will not be a part of the interface or of the IRestClientOptions it could be optional:

public static Uri BuildUri(this IRestClient restClient, RestRequest request, ParametersCollection? defaultParameters = null) {
    defaultParameters   ??= new ParametersCollection();
    var (uri, resource) =   restClient.Options.BaseUrl.GetUrlSegmentParamsValues(request.Resource, restClient.Options.Encode, request.Parameters, defaultParameters);
    var mergedUri = uri.MergeBaseUrlAndResource(resource);

    var finalUri = mergedUri.ApplyQueryStringParamsValuesToUri(
        request.Method,
        restClient.Options.Encoding,
        restClient.Options.EncodeQuery,
        request.Parameters,
        defaultParameters
    );
    return finalUri;
}

If this method will not be accessible in future it is bad for me but solvable.

Originally posted by @nesc58 in #1976 (comment)

[alexeyzimarev]

Depends on #2025

[nsulikowski]

I think this is used for logging. What about remembering the URL in a public property after the operation?

[nesc58]

For me the BuildUri makes a lot of things e.g. putting query parameters to the uri string. So it is not done by a simple remembering the URL.

I have solved the issues with a custom tracking DelegatingHandler.
The HttpRequestMessage already contains the URI result from BuildUri

So the example / pseudo codes logs and tracks the requests with times:

public class LoggingClientHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var stopwatch = new Stopwatch();
        _logger.LogDebug("Starting request to '{Request}' using method {Method}", request.RequestUri, request.Method);
        var response = await base.SendAsync(request, cancellationToken);

        _logger.LogDebug("Request to '{Request}' using method {Method} done in {Duration}", request.RequestUri, request.Method, stopwatch.Elapsed);

        return response;
    }
}

The real implementation also checks the response status codes and so on.

For me there is no longer a requirement to access the BuildUri method because I have implemented an alternative (maybe a more better/resilent way) to track the requests

[alexeyzimarev]

For me the BuildUri makes a lot of things e.g. putting query parameters to the uri string. So it is not done by a simple remembering the URL.

Looking at this, I am trying to understand what it means. Your solution for logging is great, I agree that it's a much better to do things like diagnostics and tracing. Btw, there's an open issue to support call tracing, and I think using a delegated handler would also be how I'd implement that. But, it doesn't cover "a lot of things" that you mentioned, is there anything else you needed this method for?

[nesc58]

"A lot of things" was a little bit excessive. I have used BuildUri to create the correct uri include query parameters. That's it.

With the built uri include query parameters I have logged and tracked the times.
Another point is collecting metrics using https://github.com/AppMetrics/AppMetrics per request. With these metrics I was able to check the performance of the requested resources. I have used the result from BuildUri as tag to identifier them later.

With all the information I was able to check the performance. For example I can identify network issues when the client metric is much slower than the server endpoint metrics. For external ressources I have not developed by myself it was the only thing to measure performance client side.

To find out that the DelegatedHandlers exists helped me to clean up und makes it much easier and cleaner.
I have to say "thank you" because without removing the method I've maybe never looked for alternatives :-)

[nsulikowski]

@nesc58 , could you give me more info about your LoggingClientHandler class? I'm not seeing the AddHandler method, and I'm not sure how to implement your solution. Best

[nsulikowski]

I found the hook in RestClientOptions (i.e. ConfigureMessageHandler ).
Thanks.

[alexeyzimarev]

I think all the diagnostic use cases would be solved by this #1669

You'd basically add a listener to the named ActivitySource and you can log when it starts and finishes. It also gives you the time elapsed without an extra stopwatch. Also, integrating an activity source with Open Telemetry is trivial, just use AddSource and it will work. For distributed tracing it would still be required to add the HTTP client instrumentation for remote context propagation, but it's kind of out of the box package drop anyway. I can add the headers too, but not sure if I should.

[johnfedak]

If BuildUri is not going to be restored, what is the recommended method to determine the full request URI from the clinet/request objects?

We too were using BuildUri for logging/exception messaging

[nsulikowski]

This worked for me:

public class LoggingClientHandler : DelegatingHandler
{
    readonly Action<string>? _Logger = null;
    public LoggingClientHandler(HttpMessageHandler innerHandler, Action<string>? logger = null) : base(innerHandler)
    {
        _Logger = logger;
    }

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        _Logger?.Invoke($"{request.Method} {request.RequestUri}");
        var response = await base.SendAsync(request, cancellationToken);
        return response;
    }
}

and then

        var options = new RestClientOptions("...url...")
        {
            ConfigureMessageHandler = (inner_handler) => new LoggingClientHandler(inner_handler, (s)=>Debug.Print(s)),
        };
        RestClient client = new(options, configureSerialization: cfg => cfg.UseNewtonsoftJson());

I hope it helps

[Kim-SSi]

I would love having an extension or similar.
In my case I am using the BuildUri to create the Url that is opened in a browser as part of an OAuth2 sequence.
That handled all the different parameter encoding etc.
In my case there is no actual request executed.
I could work around it by executing the request then getting the request.RequestUri.
Is there a way to execute a request as a dry run?
Or should I look into using code like the above LoggingClientHandler SendAsync and not actually calling base.SendAsync?

[nesc58]

Yeah @nsulikowski is right.
Here is the LoggingDelegatingHandler.

public class LoggingDelegatingHandler : DelegatingHandler
{
    private readonly ILogger _logger;

    public LoggingDelegatingHandler(ILogger logger, HttpMessageHandler innerHandler) : base(innerHandler)
    {
        _logger = logger;
    }

#if NET6_0_OR_GREATER
    protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var stopwatch = Stopwatch.StartNew();
        _logger.LogDebug("Starting request to '{Request}' using method {Method}", request.RequestUri, request.Method);
        var response = base.Send(request, cancellationToken);
        _logger.LogDebug("Request to '{Request}' using method {Method} done in {Duration}", request.RequestUri, request.Method, stopwatch.Elapsed);
        return response;
    }
#endif

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var stopwatch = Stopwatch.StartNew();
        _logger.LogDebug("Starting request to '{Request}' using method {Method}", request.RequestUri, request.Method);
        var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
        _logger.LogDebug("Request to '{Request}' using method {Method} done in {Duration}", request.RequestUri, request.Method, stopwatch.Elapsed);
        return response;
    }
}

And here is the initialization:

var options = new RestClientOptions
{
    ConfigureMessageHandler = handler => new LoggingDelegatingHandler(_logger, handler);
};

Like this i am able to add multiple delegating handlers by nesting them:

var options = new RestClientOptions(/*....*/)
{
   /* all other options */
    ConfigureMessageHandler = handler =>
        {
            var withLogging = new LoggingDelegatingHandler(_logger, handler);
            var withMetrics = new AppMetricsDelegatingHandler(_logger, _metricsStore, withLogging);
            // and so on
            return withMetrics;
        };
};

[alexeyzimarev]

I am waiting for more feedback about other possible use cases for public BuildUri.

[alexeyzimarev]

If BuildUri is not going to be restored, what is the recommended method to determine the full request URI from the clinet/request objects?

We too were using BuildUri for logging/exception messaging

What's your usage pattern? Chained delegating handlers seems to be a neat solution. I can also work on the activity source, which is an alternative way to receive diagnostics.

[pedoc]

Is this fixed? This is currently the only constraint preventing us from upgrading to newer versions.

[alexeyzimarev]

@pedoc fixed what? I asked for feedback about usage of BuildUrl and so far everything I received was about diagnostics, which can and should be addressed in a different way.

[pedoc]

fixed what?

Like the previous version, make BuildUrl public and allow external access.

We currently mainly use it for logging scenarios. Unlike the alternatives given above, we hope to simply record the request address and related information directly at the call site, instead of using callbacks or more complicated solutions.

[alexeyzimarev]

I am not sure why is it easier as you need to configure the logging handler once when you set up the client, and it will log everything. When you log explicitly in every place where you actually make the call, it's very easy to forget to add the logging line. Using a logging handler is very reliable, and when the RestClient will be instrumented with activity source it would be even easier, as it won't require any configuration of the client. Instead, you'd just add a logging listener to the activity source and it will log all the calls made by any RestClient instance in the system.

[alexeyzimarev]

As I wrote before, BuildUrl is implementation details, and if it's public I have to expose it via the interface, which forces me to add DefaultHeaders property to the interface, which should not be public at all.

You expect BuildUrl to return the fully constructed request URL, but it doesn't have to if it is not required by the ExecuteAsync implementation. For example, if the implementation code is changed to use the HttpClient.BaseAddress, the need for building the complete URL will disappear and the result be that the URL will be built using another function and BuildUrl will be used for compatibility purposes only. It then have a great chance to deviate and there will be no guarantee that BuildUrl will return the actual request URL. So it would be required to add tests for that specific case, etc, etc.

[alexeyzimarev]

What I can do now is to make BuildUrl public, mark it obsolete, add the activity source and document an alternative methods to do logging. Then, the next version will not get BuildUrl as public. I am ok with this as a compromise solution.

[sukaed]

Hey!
In some cases I'm using BuildUrl before sending a request. In some coupling systems it's need to generate signature with using final request URI string.
So, this method was really helpful in this case and worked great!
I can understand your wish to hide it, but many of people using it, but will never write about it here (((
Maybe you can create some separate "Helper" for this purpose?
Or additional Nuget package with extensions like this?

[AshleyMedway]

Signature generation, its important to get an exact match so nicer to ensure we use the same method for url generation in signature and for the request made

[alexeyzimarev]

Those who are interested in BuildUrl - please check the latest preview and see if it fits your needs. I plan to make a new release next week.