moleculerjs / moleculer-channels

Reliable messages for Moleculer services via external queue/channel/topic.
MIT License
72 stars 15 forks source link

sendToChannel with Context #42

Closed intech closed 1 year ago

intech commented 1 year ago

Make by analogy with the action/event in the moleculer so that params (payload) are inside the Context. Thus, we solve the problem of event tracing with internal calls in handlers. And we bring everything to a single view for all property transfer adapters inside the Context.

I understand this will cause breaking changes, but this is WIP. I don’t see anything critical for the project in this.

AndreMaz commented 1 year ago

The initial decision of not implementing the Context was to allow this module to communicate with 3rd party libs/services and, thus, facilitate the integration of moleculer into already existing projects.

Maybe we can think on a way to support both the "raw" and Context-based payloads.

intech commented 1 year ago

The initial decision of not implementing the Context was to allow this module to communicate with 3rd party libs/services and, thus, facilitate the integration of moleculer into already existing projects.

If I understand the argument correctly, then this should not break anything.

Let's discuss: If the producer is external, it simply sends to sendToChannel without knowing anything about the context.

If the consumer is external, the handler works according to the library it uses and knows nothing about the context.

If a producer or consumer is from a moleculer-channel, then it is evident that he must be part of the moleculer ecosystem and be able to context.

By the same principle, he uses a broker, which already makes him dependent on the ecosystem.

AndreMaz commented 1 year ago

I'll try to explain what I mean.

At the moment the broker.sendToChannel(<payload>) sends the <payload> without performing any changes. In this case the producer doesn't make any assumptions about the consumer, so it can be a handler registered in channels: {} or an external service that expects the <payload> as is.

On the other hand, if the broker.sendToChannel adds, under the hood, the Context then the <payload> will be modified and it will look similar to event packet. In this case, we already make an assumption about the consumer, because it should know the format of the message that it expects.

From the consumer point-of-view, registered in channels: {}, the presence of the Context also means that every message will have parentID, requestID, etc (like done here). In case of a message being sent from an external producer that's not always true.

Maybe we can add a native: true flag to the schema and opts that would mean that the payload is not generated by moleculer and does not contains ctx fields, just the payload.

intech commented 1 year ago

@AndreMaz thank you! In all supported adapters, we have native headers (only in Redis, this is a custom implementation). Moleculer-channels currently support and use them. We can pass parentID, requestID, etc. through headers, thus ensuring compatibility.

We can check for headers and recreate context at the middleware level. Thus, if we have one consumer inside the eco-system and the other is not, we will see the passage of the request inside the eco-system.

What do you think about this?

AndreMaz commented 1 year ago

Just to confirm that I understood what you're saying.

The <payload> will be the message and the Context-related params (parentID, requestID, etc.) will be placed inside of the headers field. Then, upon message reception, the channel: {} will grab the data from the headers and recreate the ctx object

Is this correct?

intech commented 1 year ago

@AndreMaz right!

icebob commented 1 year ago

Yeah, we can add context supporting without breaking, because we need to send id, parentID and requestID via headers to the consumer who can reproduce the Context from this data and continue the tracing. In this service, we implemented headers, where you can send extra meta info, and we can use it to put these values to the header...eg. with the common $ prefix to sign it's an internal field.

ujwal-setlur commented 1 year ago

I think this would be a great feature to have!

ujwal-setlur commented 1 year ago

Just to add my thoughts to this...I think this is a critical enough feature to warrant the channels feature to be a first class citizen of the moleculer world, i.e. context support should be implemented. In fact, I would argue that this should eventually just replace the events feature. I view the argument of supporting third party access and gradual introduction of moleculer to non-moleculer projects as secondary goals. Both cases can be addressed by introducing an API shim layer or proxy similar to the moleculer sidecar project. Also, I would never expose my message broker directly to a third party system to inject message directly to the bus. The only legitimate case is when the consumer is not a moleculer service, but then that service can do the extra job of ignoring the molecular specific part of the payload. My projects are pure moleculer and I plan on writing a middleware to wrap localChannel and sendToChannel inject and extract/build the context.

icebob commented 1 year ago

I've just started to work on it: https://github.com/moleculerjs/moleculer-channels/compare/master...context

Call with context:

broker.createService({
    name: "publisher",
    actions: {
        async publish(ctx) {
            await broker.sendToChannel("my.topic", { a: 5, b: "John" }, {
                ctx,
                headers: { myHeader: "123" }
            });
        }
    }
});

Context in handler

module.exports = {
    name: "sub1",
    channels: {
        "my.topic": {
            context: true,
            async handler(ctx, raw) {
                // Original `msg` in `ctx.params`
                this.logger.info("Processing...", ctx.params, raw.headers);
            }
        }
    }
}

Currently, it transfers the id, requestID, tracing, caller, level properties of ctx.

Open questions

  1. Should we transfer the ctx.meta?
  2. If yes, how we should serialize? The most adapters doesn't support nested objects in headers, only String. If we use Moleculer serializer, they are generate Buffer instead of String
  3. Should we create tracing span when we create the context before calling channel handler? Should it be the same complex like in Moleculer actions and events? E.g. params, meta settings to add values to trace spans?

Please share your thoughts.

ujwal-setlur commented 1 year ago

Yes, I would need ctx.meta since that is most commonly used to transport authorization. It will be a nested object indeed. If headers are transported as string, can we move the ctx.meta to be transported as part of the payload? However, that might be get complicated if payload is not an object. I thought headers could be an object per the documentation? I think we should create tracing spans just like action and event though this is les important to me personally.

icebob commented 1 year ago

Yeah, putting ctx.meta into the payload can cause other problems and breaks the current logic. An alternative solution if we serialize the ctx.meta with Moleculer serializers into Buffer, but putting the binary data as a base64 string into the header (buf.toString("base64")), and vice versa in the receiver side.

ujwal-setlur commented 1 year ago

I was about to suggest base64. I think this is a good approach

ujwal-setlur commented 1 year ago

Happy to test/contribute, since I need this feature! 😊

icebob commented 1 year ago

Base64 can work: https://github.com/moleculerjs/moleculer-channels/commit/8e41645941111a588a9b9a59640c68d001c2693d

ujwal-setlur commented 1 year ago

Awesome!

ujwal-setlur commented 1 year ago

@icebob I looked at the code. It looks great! I will give it a spin.

ujwal-setlur commented 1 year ago

@icebob, this works well! When can it be released? :-)

icebob commented 1 year ago

I should add tracing logic, add more tests, update docs....etc

ujwal-setlur commented 1 year ago

Any updates on this? Anything I can help with?

icebob commented 1 year ago

it looks like it's not as easy as I thought. Every adapter has different headers support, so I can't use the variable reading parts in the common code, I should change it and preprocess the headers before I call the channel handlers. It means I should change the current signature of the handlers as well, from handler(payload, raw) to handler(payload, headers, raw)

icebob commented 1 year ago

https://github.com/moleculerjs/moleculer-channels/pull/64

ujwal-setlur commented 1 year ago

Awesome!

icebob commented 1 year ago

Tracing also works fine: image

ujwal-setlur commented 1 year ago

Woohoo! Will there be a new release soon?

icebob commented 1 year ago

Yeah, this week.

icebob commented 1 year ago

It' just released

ujwal-setlur commented 1 year ago

Shouldn't the context have a property called channelName just like eventName?

icebob commented 1 year ago

good idea, please open a separate issue about it, and we can add it later.