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

[Feature]Expose the underlying http handler for consumers #92

Open
onsvejda opened this issue Feb 7, 2021 · 6 comments
Open

[Feature]Expose the underlying http handler for consumers #92

onsvejda opened this issue Feb 7, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@onsvejda
Copy link

onsvejda commented Feb 7, 2021

It is impossible to specify http message handler neither for WCF client nor for ODATA client. Exposing this can give a 3pty advantage of implementing basic policies like retry / circuit breaker on http level - instead of doing them on "higher" levels. Attaching a sample patch file how this change might look like.

0001-dev.patch.txt

@MattB-msft
Copy link
Member

Thanks for you feedback and example @onsvejda ,

We do not do this explicitly as we are changing the implementation of many of these areas actively.
we also do not usually expose these points to protect users from Protocol or API changes that may occur in the server, which is what we doing right now by transition this API from SOAP to ODATA without impacting the API surface.

Insofar as telemetry support, the CdsServiceClient ( and the CrmServiceClient) are wired up to work with the telemetry systems provided by the Dataverse server system itself. this is also an area that has seen frequent changes in the last few years.

Insofar as the support we offer,
We support client identification ( both SDK Client version and host process ), Request tracking, and Session Tracking features.
A developer has control over the Request ID's and Session ID's that the client will use for each request. SDK version and host process name are collected automatically.

You can see this information by enabling verbose mode logging in the CDS Service client, detailed logs will be emitted covering request performance, request ID's, and session ID's if present.

Is there a further level of telemetry you would like?

@onsvejda
Copy link
Author

onsvejda commented Feb 9, 2021

Hi @MattB-msft ,
I understand you currently use this extension just for telemetry - that's not what I'm after - I'm after two things:

  • consistency (right now when I have .NET core service I can specify retry / circuit breaker pattern for all the http clients via dependency injection pattern in single place) - expect for CdsServiceClient - as that one is hiding the underlying http factory construction

  • connection pooling - the other issue I raised about non-performing CdsServiceClient on k8 I was able to overcome by forking the CdsServiceClient / passing in explicitly http client factory and forcing pooling like

       CdsServiceClient.MessageHandlerFactory = new MessageHandlerFactory();
    
       private sealed class MessageHandlerFactory : IHttpMessageHandlerFactory
       {
           private static SocketsHttpHandler handler = new SocketsHttpHandler
           {
               PooledConnectionLifetime = TimeSpan.FromMinutes(10),
               PooledConnectionIdleTimeout = TimeSpan.FromMinutes(5),
               MaxConnectionsPerServer = 50
           };
    
           public HttpMessageHandler CreateHandler(string name)
           {
               return MessageHandlerFactory.handler;
           }
       }
    
    
    
  • I understand you're in the middle of switching between SOAP / pure ODATA client - still both protocols are http based so I don't see why there can't be an option to inject single factory for both of them for the cases mentioned above - i.e. if we'll use it just for pooling / auto-retry / circuit breaker / etc.

@MattB-msft
Copy link
Member

MattB-msft commented Feb 9, 2021

Thanks @onsvejda.
As I said, we are very guarded about the wire protocol and features we allow access too.
By way of explanation,
We had overexposed this in the past with our SDK's and created a substantial support issue for ourselves as both the technology and protocols evolved and were updated. While there is a population of users out there that will invest the time to understand the settings and behaviors of the transport, it is more often that exposing them leads to folks manipulating them and creating a bad state. Additionally it locks us into a given wrapper tech for the transport's that we need to map to future tech.

In this case we are challenged with supporting both full framework and .net core concurrently. The feature you are looking at here is only available in .net core. and only from 2.2 +.

With that said,

We will discus this ask and see if this is something we are willing to expose.

@mohsinonxrm
Copy link

@onsvejda , I agree with your feature request as well. Would be really nice to get access to the underlying HTTP client handler. I'm mainly looking for fault handling and logging.

@MattB-msft , I understand from your point of view as well and I get that it is an opinionated implementation, after all, it is a "client" library but perhaps there could be a way to customize or let us provide our own overrides?

@chinhpham
Copy link

I'd vote for this too. I'm implementing a batch processing job to create and update data but got different results on each test run. Retries couldn't work as expected for the same reason mentioned here.

@MattB-msft
Copy link
Member

As an update to this thread. We are still discussing providing some configuration support for Socket Handler, however we have decided against exposing the http transport at this time. This is due to many of the concerns I mentioned above and some current planning we are doing for the future of this client.

@MattB-msft MattB-msft added the enhancement New feature or request label Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants