Make ScatterGather not depend on HttpContext #4017
Conversation
|
FYI @mauroservienti @bording and I have been thinking during the migration about the complexity of the scatter gather API infrastructure. One of the changes you are making was something that we also discussed. Another one was that there is probably no need to have this inheritance chain. Most of the base functionality could be moved into a similar structure like the YARP forwarder and then having functions for merging and sorting that get plugged into it potentially hidden away on extension methods on top of that scatter gather API component. We have ultimately decided to not (yet) pursue it because it felt a bit of scope creep during the work we have done. It is good that we are now aware of these changes should we want to revisit our ideas as part of the optional backlog for the .NET 8 migration. |
|
@johnsimons looks good - can we add some tests to ensure those returned Uri values are what we expect? |
|
@danielmarbach I am pretty much done with this, the need here is to be able to call these apis without HTTP pipeline, given we are running background jobs to collect data. |
| { | ||
| static readonly IList<AuditCount> Empty = new List<AuditCount>(0).AsReadOnly(); | ||
|
|
||
| protected override string Url(AuditCountsForEndpointContext input) => |
There was a problem hiding this comment.
I can see why you are inlining the URI templates here from the perspective of your work. There is a larger implication though that comes with this change. One of the primary concept of the forwarding and the scatter gather approaches in SC is to take a request as is and forward that to the downstream instance. This is also the perspective under which we have evaluated alternative solutions like YARP for forwarding and also scatter gather (which we decided against using it there). Still the underlying premise is the same. You want to take a request and forward it down. By in lining the template creation Api class based on the data of the context, the relationship between the destination URI and the context data is now hard-coded. This means we are loosening the benefit of being able to forward ANY type of contextual information that previously was on the HTTP context. This also has consequences for any sort of header information we might want to flow across instances at a later point in time.
I do believe this should be further discussed.
There was a problem hiding this comment.
It could cause issues, given the current expectations.
I am trying to figure out if there is a way to test it, but it isn't easy.
On the other hand, we can't guarantee that the destination will have a match when we forward the request to the destination, so there is no really nice way to validate it.
Also, in theory a customer could be using different version of all instances, eg monitoring v1, SC primary v3, audit v2, I don't think we do any kind of enforcement to update everything together, so there could be mismatches.
So again, back to our requirement.
We need to use scatter-gather from non-HTTP context, and it feels weird for the caller of the API needs to know the URL path and querystrings.
So yes, maybe this proposal needs to be more discussed, I assume you referring to RFCing it?
There was a problem hiding this comment.
So @danielmarbach, after talking with @jpalac we decided to remove the URL refactor and instead just do the initial commit, which does not change the URL at all.
As far as I understand, there were no issues with it, so I am going to push ahead and merge that one so we can proceed in https://particularsoftware.slack.com/archives/C06CUKQ3668
johnsimons
force-pushed
the
john/scatter
branch
from
4ad848a
to
87e2b0b
Compare
johnsimons
force-pushed
the
john/scatter
branch
from
87e2b0b
to
d1bf96a
Compare
This is a requirement for #tf-cs3601-enable-the-platform-report-on-endpoint, where we need to use some functionality of scatter-gather from outside the HTTP context.