Add missing hooks and tracing for GraphQL operations

[NickClark]

This fixes the issues mentioned in #75

TLDR: Added hooks for context and action call options. Also made the tracing saner.

Not sure how to test the new tracing for GQL operations.

This implementation uses the Moleculer context generated from each call to the context action. This ensures a fresh context that doesn't expire despite requestTimeout being set.

I also had to create fresh contexts for every event in a subscription, otherwise, they would suffer the same fate.

In addition (and perhaps this should have been a separate MR), I add a hook for per operation call options handling. One example of why this is useful is that it will allow a developer to have custom logic determining which node a specific request my need to be sent to. Now the nodeId property can be set on the fly and potentially skip a hopping through an extra node.

[icebob]

@Hugome @shawnmcknight could you review it, please?

[shawnmcknight]

I did a review of the changes. I've got a lot of questions about flow here, but its mostly due to my lack of familiarity with subscriptions. There may be some changes warranted, but its also likely that things are correct as is. I'd ask for a little clarification on where I've had questions.

[shawnmcknight]

Forgive my lack of knowledge with subscription behaviors, so I'll probably ask questions along the way here.

Does this specific change imply that a fresh moleculer context will be created each time a subscription does to resolve, or is this the moleculer context for the entire lifespan of the connected subscription?

As far as I can tell, the primary reason for the ws action was to generate a moleculer context at connection start and bind various data to it. Presumably, that context was for the entire lifespan of the connected subscription. I'm trying to understand if the generation of moleculer context is redundant. That is, could one or the other of these calls to this.actions.xxx be handled instead with a plain old method call, or is there a benefit to establishing new moleculer Context for each of these independently?

[NickClark]

No problem, it's taking me a bit to really wrap my head around all this too, and I may be wrong, so I'm more than glad to do some mental gymnastics to make sure we get this right.

If you haven't caught up or had the opportunity recently to read my original issue You may want to do that because I go over my reasoning for a lot of this and the problems I'm trying to solve.

Apollo will create a new context for for every GQL operation. So you can actually connect, and do nothing. So the onConnect hook is important for people who want to guard that entry point. Once an operation, of any kind, is sent via WebSocket, or HTTP, then it will get a new context for that operation. We use WebSockets as our main messaging transport, we open the connection and reuse it not only for subscriptions, but also for all queries and mutations.

[shawnmcknight]

OK, so if I am understanding this correctly, the lifecycle is as follows:

• onConnect runs when the client connects. This runs a single time for the entire subscription lifecycle.
• A new Apollo Context is created each time a subscription publishes a new event, or a query/mutation is performed. This is represented in the context: integrationContext => this.actions.context(integrationContext) line.

So in each of these scenarios, an action call is being used which is establishing a new moleculer Context. In addition, this will allow consumers to wrap those actions with various moleculer lifecycle handlers if they so desire.

What I think I am still missing then is the need for the createAsyncIteratorContext method getting called. If a fresh Apollo context and a fresh moleculer context are both getting created during each operation, I'm guessing I'm misinterpreting what is considered an operation as it pertains to subscriptions and that does not include the publish event.

Assuming that I am misinterpreting this and a fresh moleculer context is needed each time a publish event is fired, I'm concerned about a few things:

1. The DataLoader cache. So I'll fill in a little more here about what I'm concerned about here. DataLoader instances are lazily loaded into the Apollo Context as they become needed. That is, every time a unique combination of action, args, and staticParams is detected, a new DataLoader instance is added to the Map inside the Apollo Context. This allows operations to be deferred and batched to limit the total number of calls during child resolution, provided that the action being called complies with the rules of DataLoader (must accept an array, must return results sequenced with that array, the output array must be the same length of the input array). One thing DataLoader does is cache results for a given "id". Therefore, if you request the same id it will return the Promise that was bound to the previous time that "id" was requested. This limits the total number of ids that need to be called. However, this cache is intended to be short lived and essentially last for a single request's lifespan. If it lasted longer than that, the cache would grow stale and potentially be out of date. There are cache invalidation mechanisms in DataLoader, but they generally just advise to create new instances on each request rather than trying to engage with the cache invalidation. If the Apollo Context is long-lived for a subscription, then that would also imply that DataLoader instances are long lived, which would result in stale caches. If the Apollo Context is short-lived then that isn't a concern, but its likely a mechanism to clear the DataLoader Map in some way (potentially at publish event time?) would be needed if the Apollo Context isn't refreshed.
2. The use of this.broker.ContextFactory.create to generate a new moleculer Context is, AFAIK, going to miss out on some things like the merging of meta. If the context or ws actions are enriching the moleculer Context with meta that is necessary for upstream actions, then that would cause an issue with those upstream operations expecting that meta to be present. Again, assuming that we need a new moleculer Context for each publish event, it might be preferable to make use of an action handler (similar to the context and ws actions) to generate the context so that it facilitates this merging properly. Alternatively, it appears to me that the Context.create method accepts an option for parentCtx, which it would utilize to do the merging of things like meta.

[shawnmcknight]

@NickClark I sent you a message on Discord to see if we can schedule some time to discuss this. It would probably be more efficient if we can do some interactive back and forth rather than trying to hash things out with this delay. Let me know if that works for you.

[shawnmcknight]

This change seems to imply that a fresh moleculer context is not being created earlier (previous comment), but is instead necessary to be created here. Is that correct?

If so, I have a few concerns. First, it does seem like the code in the server setup area is creating several instances of Context that it really cannot make use of. If they were being used then that would make sense, but if they're not it seems like a drag on connection/request time otherwise. If they are being used, then I'm not sure I'm understanding the reason to create a new Context here. Can you clarify for me?

[shawnmcknight]

My other concern here is that if we need to create a fresh moleculer context here, at action call time (and it can't be done in the server setup callback) then we probably have a (likely preexisting) issue with request to the DataLoader instances. If those instances are not being created for each new "request" (subscription, query, or mutation) then we're likely to run into caching issues on those instances. That is, repeated requests several hours later will still be using the same DataLoader cached results, which is not the prescribed "DataLoader per request" pattern.

[NickClark]

The reason for the new context here is due to the fact that these are actions fired on subscription events. So not as a direct result of the Subscription operation, but created later as a result of a pubsub event. The problem I found with using the previous context (the one created with the initial the subscription operation) is particularly evident whenrequestTimeout is enabled (which I believe should almost always be the case). The way I was looking at this is that every event is a new action source, which should create a completely new context so that it could be handled by Moleculer as such. This way things like requestTimeout work intuitively.

I don't know that DataLoader should be involved at all here, am I mistaken?

[shawnmcknight]

Can this pattern be changed to only have the call to this.actions.actionOptions a single time and assign its result to a variable that is used for all of the calls?

One other note on these options is it seems like it would need to apply to the DataLoader callback as well, so the options should probably be passed to that function and added to the call there.

[NickClark]

I think you're right.

As for DataLoader, you're probably correct there as well, I don't know much about DataLoader, so I'll have to take a look.

[NickClark]

So... I'm trying to make the change mentioned... but for some reason, calling this.actions.actionOptions earlier is magically adding {timeout: 0} to any existing params. The only change is me moving the call earlier. Does this make sense to anyone?

[shawnmcknight]

@icebob I am unfamiliar with how best to approach setting up spans for tracing, so I'm not sure if this is an appropriate pattern. Can you weigh in?

[icebob]

The method is correct but it would be good to add the typeName and fieldName into tags, as well.