alpacahq / alpaca-trade-api-csharp

C# SDK for Alpaca Trade API https://docs.alpaca.markets/
Apache License 2.0
252 stars 83 forks source link

[FEATURE]: Logging #605

Closed walterX12 closed 1 year ago

walterX12 commented 1 year ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe.

It would be very helpful if the library provided logging. It would help troubleshooting. I checked source codes and there is no logging at all.

Am I missing something ?

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Anything else?

No response

OlegRa commented 1 year ago

Hi @walterX12 , thanks a lot for this proposal. You are right - there is no logging support in this SDK for several reasons:

So I need input from your side for a better understanding of your needs:

  1. Which information do you want to see in the log messages?
  2. Do you need logging for both REST and WS clients?

In any case, I'll add this feature only in the next major SDK release because I need time for design and testing.

walterX12 commented 1 year ago

Hi,

I see your points. What I am trying to achieve, to be able to track down as much details of any issue, which might occur in REST requests during development and in later in production, to be able to:

In other words, it is very crucial to be able to figure out root cause from logs, to be able to improve application without "guessing", what happened on HTTP(s) layer.

It would be helpful if I can add HttpMessageHandler derived from System.Net.Http.DelegatingHandler to Alpaca HTTP Client somehow. Is it possible ? Am I missing something ?

I am connecting to several other REST APIs ad I am logging request/response via

builder.Services.AddHttpClient("HttpMessageHandler")
    .AddHttpMessageHandler<HttpResponseLoggingHandler>();

HttpResponseLoggingHandler

public class HttpResponseLoggingHandler : DelegatingHandler
{

    protected static Logger log = LogManager.GetCurrentClassLogger();

    protected override async Task<HttpResponseMessage> SendAsync(
        HttpRequestMessage request, CancellationToken cancellationToken)
    {
        log.Info(request.ToString());
        if (request.Content != null)
        {
            var requestPayload = await request.Content.ReadAsStringAsync();
            log.Info($"Request payload:{requestPayload}");

        }

        HttpResponseMessage response = await base.SendAsync(request, cancellationToken);

        log.Info(response.ToString());
        if (response.Content != null)
        {
            var responseContent = await response.Content.ReadAsStringAsync();
            log.Info($"Response payload:{responseContent}");
        }
        return response;
    }
}

It would not add dependency to your code and projects focusing on really high performance (sacrificing detaialed logging) , can use conditional loggers like log.ConditionalTrace , which deletes logging from Release build.

Thanks for your help.

OlegRa commented 1 year ago

I believe you know about the SDK extensions package - it contains helper methods for creating strongly-typed HTTP clients. The main problem with the current approach is the fact that all these methods return the IServiceCollection interface so you are unable to extend the created strongly-typed HTTP client with a custom handler. I see the next possible solutions for this problem:

  1. Change signatures and return IHttpClientBuilder from all AddAlpaca...Client extension methods. This is a breaking change but I'm not sure it was a good idea to return IServiceCollection interface and the amount of potential changes on the client side is minimal.
  2. I can add generic overloads for existing methods that will allow users to provide a type of additional HTTP handlers. This one is also breaking change (at least at compile level) and solves only one problem instead of 1st approach that provides a more flexible solution.
  3. Add overloads that take delegate for configuring IHttpClientBuilder - this is a non-breaking approach but it requires a lot more code to be written and will make signatures more "cryptic" for most users.

I personally like the 1st solution and I can make this change even in the upcoming 6.2.0 alpha release of the Extensions package so @walterX12 you'll be able to test it and provide your feedback.

walterX12 commented 1 year ago

Hi,

I prefer 1st solution and I believe that it is very elegant suggestion/solution. I believe that anyone who is is fluently using return value IServiceCollection ( from AddAlpaca...Client extension methods ) at current code, can easily overcome/solve this.

OlegRa commented 1 year ago

@walterX12 You can test this approach in the Extensions 6.2.0-alpha1 release already available on NuGet. Please, try and provide your feedback.

walterX12 commented 1 year ago

Thank you for quick enhancement. I tested and it works as expected. Thank you.

I checked all my use case when AlpacaClients are created and I am creating clients dynamically because I support multiple user using my app. Each user can provide its own API Key/Secret ( for both Paper and Live environment ) and I create for each user their own client(s) instances. E.g.:

cryptoClientLiveForUserXy = Environments.Live.GetAlpacaCryptoDataClient(new SecretKey(apiKey, apiSecret))

Would it be possible to derive all Rest api clients ( like IAlpacaCryptoDataClient , IAlpacaTradingClient ...) from one interface supporting AddHttpMessageHandler method ? It would not break any existing method, constructors signatures and it would enable to add HTtpHandler if needed.

OlegRa commented 1 year ago

@walterX12 I understand your use case but the proposed solution is not easy to implement. The current approach is based on read-only instances of the HttpClient class stored inside the client and I don't want to change it just for this use case. I think a better approach is to extend the existing configuration objects used for the client's initialization with the ability to provide custom HttpHandler type(s) instead. I'll finalize design next week.

walterX12 commented 1 year ago

Ok. It would help.

OlegRa commented 1 year ago

@walterX12 You can test changes in the upcoming 6.0.2-beta2 release. I've added an extension method for the AlpacaClientConfigurationBase inheritors named WithHttpMessageHandlerFactory that should solve your problem.

Expected usage pattern (I've not tested this snippet on my side so treat it as a pseudo-code):

cryptoClientLiveForUserXy = Environments.Live.GetAlpacaCryptoDataClientConfiguration(new SecretKey(apiKey, apiSecret))
    .WithHttpMessageHandlerFactory(handler => new LoggingHttpMessageHandler(logger) { InnerHandler = handler })
    .GetClient();

Please, check if this approach work or not and provide your feedback here.

OlegRa commented 1 year ago

I will close it for now. @walterX12 if you still have some questions/ideas, please, open another issue.

walterX12 commented 2 months ago

Sorry for a late reply. I had to put the project on hold for a while. I have tested .WithHttpMessageHandlerFactory and it works as expected. Thank you for implementation.