Implement header conversion between AMQP 1.0 and 0.9.1

[luos]

Proposed Changes

Implements optional conversion of AMQP 1.0 headers to 0.9.1 headers for some headers types (utf8, uint, bool). Implements rabbitmq#2591

Implements optional conversion of AMQP 0.9.1 headers to AMQP 1.0 App Properties.

I changed the conversion to use rabbit_msg_record though moving all this messaging conversion I am not sure it's possible.

This is picked from #4833.

Let me know what you think.

Types of Changes

What types of changes does your code introduce to this project? Put an x in the boxes that apply

• [ ] Bug fix (non-breaking change which fixes issue #NNNN)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• [ ] Documentation improvements (corrections, new content, etc)
• [ ] Cosmetic change (whitespace, formatting, etc)
• [ ] Build system and/or CI

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask on the mailing list. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• [x] I have read the CONTRIBUTING.md document
• [x] I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• [x] I have added tests that prove my fix is effective or that my feature works
• [x] All tests pass locally with my changes
• [ ] If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
• [ ] If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

[mergify[bot]]

This pull request modifies the bazel build only. If it is a deps update or app_env change, remember to sync any changes to the makefiles.

[luos]

I think conversion can be quite resource intensive and unnecessary, ie when only AMQP 1.0 clients are communicating. Not sure if it's worth adding it always.

[michaelklishin]

We are weighing the cost (per message) of such conversion vs. the cost of an ETS read to test if the conversion is enabled. I won't be able to tell what carries greater overhead.

We can collect some data and decide. Targeting the case with only AMQP 1.0 clients communicating makes sense to me.

[luos]

Hi,

Yes, so I was meaning that the conversion is wasteful for AMQP 1.0 only clients as the result will be thrown away, as it always uses the AppPropsBin for those from the AMQP 0.9.1 header.

I tried to run perf, but sadly all functions came out as unknown, probably because I did not compile Erlang on the machine but it was from a package. I will try again as soon as I can.

Are you OK with running perf yourself?

Let me know if there are other changes needed for this PR.

[luos]

Hi,

Do you think there is anything I could do to push this forward?

Thanks

[michaelklishin]

Understanding the effects of this on throughput is the primary thing left.

[luos]

Hi,

I've made some performance tests. Seems like the cost of converting the headers overshadows everything. For this reason, it may be a good idea to leave this as a flag if someone does not need the conversion it can drastically slow down the incoming messages.

I guess one improvement could be made, which is store it in the state if conversion is enabled or not. I'd hope that's faster than ets. :)

The test is for converting AMQP 1.0 Application properties to AMQP 0.9.1 headers. The test code and headers you can see here. The only changed between runs were the RabbitMQ config / code part. Except for the "Nothing to convert" tests where no app headers were added at all.

https://github.com/luos/amqp10client/blob/b973c349ba75c4a55bc027bac44e95126db338d7/src/amclient_client.erl#L206-L215

	Message Count	Msgs / sec	Total time
Disabled (no config)	300000	39751	7546.866
Disabled config	300000	42571	7.047
Disabled config	300000	37397	8.022
Enabled 1	300000	17140	17.503015
Enabled 2	300000	17227	17.41474
Compiled out (disabled)	300000	34672	8.652493
Compiled out (disabled)	300000	35971	8.340113
Compiled out (disabled)	300000	25420	11.8019
Compiled in (enabed) (whithout ets lookup)	300000	18868	15.900334
Compiled in (enabled) (whithout ets lookup)	300000	16291	18.415505
Nothing to convert enabled	300000	47693	6.290289
Nothing to convert enabled	300000	42762	7.015538
Nothing to convert (compiled out)	300000	48823	6.144593

[michaelklishin]

There are two "Nothing to convert enabled" rows, was one of them mean to say "disabled"?

[luos]

For all cases, it's the same test running multiple times. Sorry for the confusion.

Compiled in/out means I just replaced the macro with true/false.

Edit: Sorry, misunderstood. No, I just figured if it there is nothing to convert then there is no need to test both. :-)
The last two tests (Nothing to convert enabled vs compiled out) are more or less the cost of the ets lookup.

[luos]

Hi! Is there anything I could add to this to be mergeable? Thanks

[kjnilsson]

I think this is ok. The formatting is still a bit off but we are likely to completely revisit the entire 1.0 plugin at some point so it is fine for now.