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

Refactor RequestData #127

Merged
merged 12 commits into from
Oct 31, 2024
Merged

Refactor RequestData #127

merged 12 commits into from
Oct 31, 2024

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Oct 30, 2024

This PR is meant as the first step to undo making RequestData the God Object it is today.

  • RequestData is now completely readonly
  • PostData is moved outside RequestData
  • HttpMethod and Path and RequestParameters now live outside RequestData, more info on this later.
  • OpenTelemetryData now lives outside RequestData.
  • Fixes several instances of boxing in HeaderList constructors.

This now also introduces 2 new records.

EndpointPath a readonly record struct that instructs what API we're calling.
Endpoint takes and EndpointPath and Node to take ownership of Uri creations.

This is hopefully a first step where we can cache these in calling libraries.

  • TransportConfiguration now implementents IRequestConfigation

The goal here is for RequestData to turn into BoundConfiguration and instance we can cache per TransportConfiguration. Unless a local per request configuration is provided in which case we'll need to new a new instance which is rare.

This refactoring introduces an `Endpoint` parameter to various request methods across the codebase, ensuring a more consistent and descriptive way to handle request data. It modifies the necessary invocations and internal handling, enhancing clarity and maintainability.
Refactored methods and constructors to use `EndpointPath` instead of individual parameters for HTTP methods and paths. Improved encapsulation of path construction and method handling in various transport request invocations.
Enhanced `Request` and `RequestAsync` methods across various classes to accept a new `PostData` parameter. This improves the handling of POST data in requests. Also, included nullable reference type annotations and updated related tests.
Refactored PostData methods to include disableDirectStreaming parameter for better control over streaming behavior. Removed unused variables and streamlined RequestData construction to ensure settings are correctly assigned. Improved consistency in handling streams and memory buffers across synchronous and asynchronous write operations.
Update `PipelineException` constructors to use nullable inner exceptions and refine exception handling logic in `DefaultRequestPipeline`. Remove unnecessary `MadeItToResponse` flags and related properties in `RequestData` and invokers for streamlined code.
Modified methods to include a boolean isAsync parameter for more precise handling of asynchronous requests. Removed IsAsync property from RequestData.
Simplified `RequestData` instantiation by removing the `OpenTelemetryData` parameter across the codebase. Adjusted related properties and methods to maintain functionality and align with the new constructor signature.
@flobernd
Copy link
Member

flobernd commented Oct 30, 2024

Without having looked at it yet:

Unless a local per request configuration is provided in which case we'll need to new a new instance which is rare

Maybe we can find a way to avoid the allocations. I need the UserAgent to be present in the IRequestConfiguration in order to override it in high level integrations that allow users to pass in a custom instance of the client. In these cases, we can't modify the user agent in the ITransportConfiguration without affecting the whole client instance.

This would cause the allocation to happen for every single request.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Oct 30, 2024

There are two pieces a global TransportConfiguration and a local per request RequestConfiguration.

The goal although not yet implemented is that if TransportConfiguration is provided (one exist per client) we never have to allocate more then 1 RequestData for all requests. This is 90% of the time the case.

Do you need to specify a different UserAgent per request/api?

@flobernd
Copy link
Member

The goal although not yet implemented is that if TransportConfiguration is provided (one exist per client) we never have to allocate more then 1 RequestData for all requests. This is 90% of the time the case.

Sounds great 👍

Do you need to specify a different UserAgent per request/api?

That's not the case, no. But I still need a way to override the UserAgent for every single request. This is beacuse in some high level integrations like e.g. the Semantic Kernel connector, we allow the user to pass an already initialized ElasticsearchClient instance to the integration (this is a pattern that we can't easily change as it's essential in a lot of scenarios like e.g. in a dependency injection container).

This client instance is internally used to perform the requests. We want to use a custom UserAgent here for tracking purposes, but at the same time we do not want to modify the "global" TransportConfiguration, because that would mess with the UserAgent on all other places where the user uses the same client instance.

My idea was to add UserAgent to IRequestConfiguration for this purpose.

Your refactoring is a good improvement anyways for all other cases where we do not have to override on a per-request basis. Maybe we can be smart in some way to optimize the per-request-override case as well.

@stevejgordon
Copy link
Collaborator

That's not the case, no. But I still need a way to override the UserAgent for every single request. This is beacuse in some high level integrations like e.g. the Semantic Kernel connector, we allow the user to pass an already initialized ElasticsearchClient instance to the integration (this is a pattern that we can't easily change as it's essential in a lot of scenarios like e.g. in a dependency injection container).

Would it make sense to consider the client passed to Semantic Kernel dedicated to that use, i.e. create an instance with the configured UA etc. that is only used by Semantic Kernel. If the app also needs to use a client for other ES-related code, they could have a second instance of the client configured for that use case. Although there is some extra overhead, this might be the rare use case where having more than one client in an app is reasonable?

Copy link
Member

@flobernd flobernd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a first glance 🙂

Copy link
Collaborator

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too. Thanks for starting off this improved direction!

@Mpdreamz Mpdreamz merged commit d5141a9 into main Oct 31, 2024
5 checks passed
@Mpdreamz Mpdreamz deleted the refactor/request-data branch October 31, 2024 12:03
@flobernd flobernd added the v0.5.0 label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants