Particular / NServiceBus

Build, version, and monitor better microservices with the most powerful service platform for .NET
https://particular.net/nservicebus/
Other
2.07k stars 651 forks source link

Allow custom MessageID generator now fixed by comb guid #4236

Open ramonsmits opened 7 years ago

ramonsmits commented 7 years ago

Although the message ID can be set using the send options it would also make sense to have the ability to override this:

This would allow the transport to override this generator and can use the format that is best and native to the transport.

bording commented 7 years ago

This would allow the transport to override this generator and can use the format that is best and native to the transport.

Can you provide some more details as to what scenarios you're thinking this would enable? What transports would this benefit? For example, I can't think of any reason RabbitMQ would care about this.

ramonsmits commented 7 years ago

Comb guid is used with SQL storage in mind as this is 'clustered index friendly' as records can then be appended at last data page.

I wondered why we have this decision fixed in the generation of the message ID.

Indeed, thinking about it it probably doesn't really matter from a transport perspective but then why isn't it they just Guid.NewGuid().

ramonsmits commented 7 years ago

I see we do the same for the saga ID : https://github.com/Particular/NServiceBus/blob/develop/src/NServiceBus.Core/Sagas/SagaPersistenceBehavior.cs#L266

Although that probably makes more sense for SQL it probably doesn't have any effect on Raven or on Azure persistence for that matter.

timbussmann commented 7 years ago

I thought raven is also no fan of Guid.NewGuid from an indexing perspective?

so far I'm not really seeing what value Guid.NewGuid() would provide?

danielmarbach commented 7 years ago

Why do you think this is relevant for control messages?

danielmarbach commented 7 years ago

There is an issue to make the saga id pluggable:

ramonsmits commented 7 years ago

@danielmarbach Don't know if it makes sense from an control message perspective as those are our own messagas but yes I should have referenced the extendable options instead:

https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.Core/Extensibility/ExtendableOptions.cs#L17

Updated the description with that link.

andreasohlund commented 7 years ago

I'm with @bording here. Is there really a use case for this from a transport perspective?

Remeber that we transfer the message id as a header so the transport is free to use whatever native id it seeS fit. Eg msmq uses its own id(no way to control the msmq id). We also talked about having sql use IDENTITY instead of the combguid we pass.

The only thing that comes to mind in terms of controling the message id would be the outbox since that id is used as the key?

So perhaps this is the use case?

ramonsmits commented 7 years ago

@andreasohlund In V6 the message ID can only be set via the sendoptions. If this is plugable than the ID can also be generated based on 'context'. For example, the incoming message context could be used to help generating a deterministic message ID that can be helpful for idempotent processing.

timbussmann commented 7 years ago

to find some outcome for this discussion: so far there were no objections of such an API being possible, but we haven't found a real use case for it. Sounds like we should close this (and discuss this again once we have a specific use case)?

andreasohlund commented 7 years ago

For example, the incoming message context could be used to help generating a deterministic message ID that can be helpful for idempotent processing.

It seems like we agree that this would only be relevant for the outbox since that's the only thing that uses message-id for deduplication (I think the gateway does as well, need to check). Should we consider adding a way for the user to provide the "deduplication id" to the outbox instead of changing the message id?

.Outbox().CustomDeduplicationId(context=>{
    return FigureOutTheid(context);
})

or perhaps on the sending side:

var options = new SendOptions();

options.OutboxDedupeId($"placeorder-{order.OrderId}");

context.Send(new PlaceOrder(...),options);
ramonsmits commented 7 years ago

@andreasohlund Any customer based dedupe should also be using the message ID and I use message ID too in my dedupe using azure storage https://github.com/ramonsmits/NServiceBus.Deduplication

I don't see what the outbox has to do with this as you also would like to do this when using immediate dispatch. The ID is generated very early in the send pipeline as it can be referenced by any outgoing behaviors and be used for any actions as that ID is fixed or is this assumption wrong?

How is the snippet any different than manually setting the message ID via SendOptions?

var options = new SendOptions();
options.SetMessageId($"placeorder-{order.OrderId}");
andreasohlund commented 7 years ago

How is the snippet any different than manually setting the message ID via SendOptions?

Fair point.

So in essense we're saying that the use case is:

Deduplication logic, either custom written by our users, or by us like the outbox should use the message id and in order to provide either a business related ID or an ID that is suitable for the storage used we should make the id generation scheme pluggable.

This is true today since the COMB Guid used is very suitable for Sql but the worst possible ID for the RavenDB outbox.

@Particular/nservicebus-maintainers if you agree I will raise an issue to track this action point?

timbussmann commented 7 years ago

provide either a business related ID or an ID that is suitable for the storage used we should make the id generation scheme pluggable.

that sounds like there could be conflicts by different parties trying to have different message id strategies registered?

I'm not really sure there is enough value to change the message id for the sake of the outbox. Is there any way the outbox could make use of a custom header which provides a more outbox-friendly "alternative id"?

dbelcham commented 7 years ago

@ramonsmits @bording @timbussmann @andreasohlund Was a final decision ever made on this?

timbussmann commented 7 years ago

I'm having difficulties to see exactly who would be responsible or capable of defining those ID generation strategies in the first place. There may be different interests from persistence, transport, outbox and potentially other features. I'd prefer an approach which enables to provide custom ids additionally to the message instead of replacing the message id generation strategy. I vote to close this issue.

danielmarbach commented 7 years ago

Having a hard time to actually follow what is proposed here. We all agreed that it is possible to set the message id on the send options. Is the problem that this is the only way to set the message id and it might make it a bit cumbersome? If the answer is yes then what would hinder a user or a plugin to hook into the outgoing logical part of the pipeline and have a strategy based on the message type?

I don't see enough value here to keep it open since we have all the moving pieces to enable such a scenario haven't we?

ramonsmits commented 7 years ago

@danielmarbach AFAIK that isn't possible anymore in V6 as the ID is set immediately and cannot be changes later in the pipeline.

dbelcham commented 7 years ago

is this related to:

danielmarbach commented 7 years ago

@Particular/sqlserver-transport-maintainers @Particular/rabbitmq-transport-maintainers @Particular/azure-maintainers do you see value in this proposal? If yes should we raise an issue in platform dev? If no we are leaning towards closing this.

tmasternak commented 7 years ago

With my @Particular/sqlserver-transport-maintainers 🎩 on I don't see value.

In most of the cases SqlServer transport does not really care/interact with message ID managed by Core. The only exception that I know about is when message does not have MessageID header value and than native message ID becomes Core's ID but that is irrelevant for this discussion.

bording commented 7 years ago

@danielmarbach RabbitMQ doesn't care about Message IDs at all, and doesn't even require one to send a message, so the choice of how NSB generates its ID doesn't matter.

SeanFeldman commented 7 years ago

Currently we ignore Message ID and set a arbitrary Transport Message ID. If there will be a way to connect message ID generation into user code (i.e. the message ID to be generated from the message business data), I do see a potential value for ASB customers. It would allow to take advantage of the native duplicate detection. If generating message ID from message data is not possible, then there's not real value for ASB. /cc @Particular/azure-maintainers