Noticing a couple things with `FlowSubscriptionExe...

# graphql-kotlin

[Joe]

Noticing a couple things with `FlowSubscriptionExecutionStrategy`:
1. it now returns a Publisher now instead of a Flow. This makes sense for spring webflux, but we're consuming `Flow`s directly and the Flow->`Publisher`->`Flow` round trip has caused resource leak problems for us in the past. Would something like a NativeFlowSubscriptionExecutionStrategy that returns `Flow`s be welcome in graphql-kotlin or should we maintain it externally?
2. instrumentation doesn't appear to be applied to subscription results. Previously fixed in https://github.com/ExpediaGroup/graphql-kotlin/pull/742 but maybe in going to Publisher lost that? (or maybe something's changed in graphql-java 16 around it?)
other than that, updating package import definitions and some renames in graphql-java 16 (ExecutionPath -> ResultPath) appear to be the main changes that we need to adjust for. There's also some weirdness with the selectionSet duplicating fields that appears to be filed upstream as https://github.com/graphql-java/graphql-java/issues/2275.

[Dariusz Kuc]

If I remember right we tried using flows for subscriptions but it was causing some issues

[Dariusz Kuc]

I'll try to dig up corresponding prs

[Joe]

ok. I'll plan on maintaining NativeFlow version in our own repos for now. I'm surprised by (2) since there's a test for instrumentation being applied, but did find that result.getData<Publisher<ExecutionResult>>().asFlow().onEach { it.... }.....collect() is missing the instrumented values in the it ExecutionResult.

[Dariusz Kuc]

ah yeah so re #1 -> I've updated it to publisher when integrating with spring server as current subscription beans were based on flux (https://github.com/ExpediaGroup/graphql-kotlin/pull/972)

[Dariusz Kuc]

ideally we should rewrite those spring handlers to use flows but didn't have time to do it

[Joe]

Yeah makes perfect sense from an already existing flux perspective

[Dariusz Kuc]

re: 2 I just checked the unit test and extensions gets populated as expected (https://github.com/ExpediaGroup/graphql-kotlin/blob/master/generator/graphql-kotli[…]ql/generator/execution/FlowSubscriptionExecutionStrategyTest.kt), unsure what might be causing different behavior

[Joe]

ok digging a little deeper on (2) it appears that the Instrumentation is being applied, but the coroutine context is not preserved between the raw flow and the converted publisher. Specifically we return a flow.flowOn(MDCContext()) and then attempt to pick up the MDC value from our Instrumentation to make a RequestId accessible. Will try to create a simple reproduction now that I've figured that out.

[Dariusz Kuc]

ah I think it is most likely due to the flow -> publisher change if you use the default flow subscription strategy from gql kotlin

[Dariusz Kuc]

i.e. publisher has no concept of contextual data

[Joe]

Yeah, for our specific case it looks like you could do (publisherOrFlow as? Flow<Any>)?.asPublisher(MDCContext()) but I don't see a good way to do this generically (ie. inherit context that's already there in the calling context, since we've bridged into java/non-coroutine land at this point?). sticking with avoiding flow -> publisher -> flow conversion is the way we'll go.

[Joe]

After a little more thought put this together: https://github.com/ExpediaGroup/graphql-kotlin/pull/1116 not sure if you'd want it or not, but figured worth sharing some thoughts on it.