rabbitmq / rabbitmq-server

Open source RabbitMQ: core server and tier 1 (built-in) plugins
https://www.rabbitmq.com/
Other
12.29k stars 3.91k forks source link

Timestamp is int32 on a received trace message #2991

Open bollhals opened 3 years ago

bollhals commented 3 years ago

Copy from https://groups.google.com/g/rabbitmq-users/c/YipXsgD7XpM

We've setup the vhost tracing to queue all published messages into a separate queue. When consuming messages from said queue in dotnet (probably irrelevant), the timestamp of the original message is delivered/read as int32. Shouldn't this be a long/int64?

It seems to me (I don't really know the language, so I can be wrong) that this line is responsible for the incorrect forwarding.

=> This issue prevents us from sending a timestamp with ms resolution instead of s, as the resulting long value is > 32bit value.

michaelklishin commented 3 years ago

The timestamp field in AMQP 0-9-1 has a a second resolution, not ms. Which is why https://github.com/rabbitmq/rabbitmq-event-exchange/commit/97a0ac7d3e023688281ae47cc91885226132a3b7 and IIRC a couple of similar changes were introduced. The same can be done to/in tracing.

michaelklishin commented 3 years ago

Ah, I forget that we only use message headers in tracing events. So you may be right, it can be a matter of using an unsigned type since timestamps can't be negative.

michaelklishin commented 3 years ago

So the type itself is not going to help much. We have to use a new header, timestamp_in_ms, much like we do with Shovel.

bollhals commented 3 years ago

I think I confused you a bit by the ms topic. My bad.

In the C# client it passes the timestamp as AMQPTimestamp, which is serialized as a long value.

My company is investigating the possibility to sending the timestamp value as ms instead of s. => the actual value passed is basically just 1000x higher, as seen here in "Timestamp in milliseconds" instead of "Epoch timestamp"

This causes it currently to overflow due to the int32 size used when we look at tracing messages

bollhals commented 3 years ago

Also for normal message delivery this works flawlessly, as there the value is transmitted as long(64bits) as it was sent.

michaelklishin commented 3 years ago

@bollhals OK, that was more or less my understanding. I got a little confused because I used a different client to observe.

I can produce a one-off build of this plugin that uses long for value type for you to try with your actual apps by replacing the rabbitmq_tracing.ez under the plugins directory of your installation. Would that work? What Erlang version should I compile for?

bollhals commented 3 years ago

That would certainly work๐Ÿ‘๐Ÿผ I'll have to check tomorrow to be sure.

michaelklishin commented 3 years ago

@bollhals can you please confirm using Wireshark that the value is not truncated by the .NET client? Or put together a small runnable example I can use to investigate?

bollhals commented 3 years ago

I can put together an small runnable if desired, but you'd have to have a server setup as well or I'd have to spin one up as part of the example.

On the other hand, the .NET client does the following while reading the values for the message dictionary So if the server sends a proper long value, it would transmit a 'l' and not an 'i'. From that code, the only possiblity where we end up with an int32, is when the server sent an 'i'.

PS: To be even more "correct", the server should probably send 'T' for a timestamp :)

bollhals commented 3 years ago

What Erlang version should I compile for?

I've checked and we're using the latest RabbitMQ version 3.8.14 container from dockerhub. This means Erlang 22.3 if I'm not mistaken.

deadtrickster commented 3 years ago

@bollhals is this still relevant for you?

michaelklishin commented 3 years ago

Since this is Firehose-only, we can consider changing the type for 3.9 https://github.com/rabbitmq/rabbitmq-server/pull/2995. But the original timestamp is in seconds per the AMQP 0-9-1 spec, I'm afraid. Which should be a limiting factor for any client library.

We can still add a new field, timestamp_in_ms, like we did in Shovel IIRC.

bollhals commented 3 years ago

@bollhals is this still relevant for you?

We've found other ways to get what we needed, so no it is not anymore.

Since this is Firehose-only, we can consider changing the type for 3.9 https://github.com/rabbitmq/rabbitmq-server/pull/2995.

Since it is still a bug, I think it should be changed in 3.9 ๐Ÿ‘๐Ÿผ