Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RabbitMQ x-death header documentation #2688

Closed
ansd opened this issue Apr 19, 2024 · 8 comments
Closed

Fix RabbitMQ x-death header documentation #2688

ansd opened this issue Apr 19, 2024 · 8 comments

Comments

@ansd
Copy link

ansd commented Apr 19, 2024

The Spring Cloud Stream RabbitMQ Binder Reference Guide recommends that the AMQP 0.9.1 client re-publishes a message with the x-death header it received from RabbitMQ in order to determine the number of times (count) the message was received and re-published to RabbitMQ, see rabbitmq/rabbitmq-server#10709 (comment)

We consider this an anti-pattern or even bug because AMQP 0.9.1 headers prefixed with x- including x-death "belong" to the broker and are not meant to be specially parsed and interpreted by the broker when received from an AMQP 0.9.1 client.

Hence, as explained here, we recommend changing the documentation to instead use some custom non x-header to determine how often a message was re-published from the client to the broker.

Starting with RabbitMQ 4.0, the current documented pattern by Spring Cloud Stream RabbitMQ Binder Reference Guide will break.

@artembilan
Copy link
Member

I'm not fully sure what you find as a wrong recommendation. I don't see any sentence in that doc which says something like re-publishes a message with the x-death header it received from RabbitMQ.
It indeed looks like we rely on the x-death to determine the count and sometimes an original exchange/routing-key, but no where we say explicitly that this header has to be set by the client code.

I think we just followed Dead Letter Exchanges docs to deal with expectations after DLX.

Thanks

@ansd
Copy link
Author

ansd commented Apr 19, 2024

Thanks @artembilan for the clarification.
I got misled by the comment from @dvovney in rabbitmq/rabbitmq-server#10709 (comment)
I close this issue for now.

@ansd ansd closed this as completed Apr 19, 2024
@artembilan
Copy link
Member

Apparently x-death.count is not going to be incremented from now on. Therefore we indeed have to come up with other strategy in the ConditionalRejectingErrorHandler.
Plus fix docs respectively here and there in Spring Cloud Stream.
See more info in the: spring-cloud/spring-cloud-stream#2939

@artembilan
Copy link
Member

@ansd ,

Would you mind, please, confirm that x-death header is still going to be present in the consumed message?
Apparently we have a logic like this:

	/**
	 * Set to false to disable the (now) default behavior of logging and discarding
	 * messages that cause fatal exceptions and have an `x-death` header; which
	 * usually means that the message has been republished after previously being
	 * sent to a DLQ.
	 * @param discardFatalsWithXDeath false to disable.
	 * @since 2.1
	 */
	public void setDiscardFatalsWithXDeath(boolean discardFatalsWithXDeath) {

Apparently we don't deal (and don't talk) about its count property anywhere in Spring AMQP.

Thanks

I probably won't mind to implement some new strategy for header different from x-death, but it looks like for now we have to just fix docs in Spring Cloud Stream as you have talked originally.

@samragu
Copy link

samragu commented Apr 24, 2024

Starting with RabbitMQ 3.13, the count parameter in x-death header is no longer incremented. This breaks SCSt applications that relied on this header and parameter to keep track of retry attempts. Note that spring-amqp/SCSt already sets another header x-exception-stacktrace with Java exception stacktrace that resulted in the message being dead lettered. We will need another similar header, for example, x-retry-attempt to carry the retry attempt (i.e., number of times the message is published to DLQ). Note that RabbitMQ is discouraging usage of x- prefixed headers, so both x-exception-stacktrace and x-retry-attempt should be renamed to something else

@artembilan
Copy link
Member

I see. Would it be OK to have them as something like spring-exception-stacktrace and spring-retry-attempt respectively?
We have a number of others in the RepublishMessageRecoverer:

	public static final String X_EXCEPTION_STACKTRACE = "x-exception-stacktrace";

	public static final String X_EXCEPTION_MESSAGE = "x-exception-message";

	public static final String X_ORIGINAL_EXCHANGE = "x-original-exchange";

	public static final String X_ORIGINAL_ROUTING_KEY = "x-original-routingKey";

@ansd
Copy link
Author

ansd commented Apr 25, 2024

@artembilan

Would you mind, please, confirm that x-death header is still going to be present in the consumed message?

Yes, the x-death header including its count field will be present in a consumed message (if this message got previously dead lettered).

The only thing that changes in RabbitMQ 3.13 is that RabbitMQ (upon receiving) won't interpret anymore the x-death header when a client (re)publishes a message.

So, if there is any code or documentation within Spring that causes messages being published to RabbitMQ to have the x-death header set, this will need to change.
If there is no such code or documentation, nothing within Spring needs to change.

@artembilan
Copy link
Member

Yeah... Nothing to fix here.

@artembilan artembilan closed this as not planned Won't fix, can't repro, duplicate, stale May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants