grafana / xk6-amqp

A k6 extension for publishing and consuming messages from queues and exchanges using AMQP 0.9.1.
Apache License 2.0
25 stars 28 forks source link

Add messagepack encoding #17

Closed PabloDelBarrioArnanz closed 1 year ago

PabloDelBarrioArnanz commented 1 year ago

PR to add messagepack encoding for rabbit message, this functionality can be activated indicating that the contentType of the message is 'application/x-msgpack' any other type must work as usual.

Json2msgpack library is used for marshalling.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

pablochacin commented 1 year ago

Hi @PabloDelBarrioArnanz I'm not sure that adding explicit support for content type encoding in a transport extension is the best approach. Imagine how this code would evolve if we keep adding support for more content types:

    if options.ContentType == messagepack {
        publishing.Body = json2msgpack.EncodeJSON([]byte(options.Body))
    } else {
        publishing.Body = []byte(options.Body)
    }

I'm therefore wondering why not make the encoding/decoding in JS either using an existing JS library (in a quick search I found this) or by implementing the msgpack encoding/decoding in its own extension (say xk6-msgpack)

PabloDelBarrioArnanz commented 1 year ago

Sorry for the extreme delay...this was lost in my Inbox.

I'm not so sure we should accept this as the github.com/izinin/json2msgpack does not have any licensing information associated with it.

Hello @javaducky!! True, I did not notice that it does not have a defined license, maybe if I used this implementation, would you accept the feature? 😄 => https://github.com/vmihailenco/msgpack

PabloDelBarrioArnanz commented 1 year ago

Hi @PabloDelBarrioArnanz I'm not sure that adding explicit support for content type encoding in a transport extension is the best approach. Imagine how this code would evolve if we keep adding support for more content types:

  if options.ContentType == messagepack {
      publishing.Body = json2msgpack.EncodeJSON([]byte(options.Body))
  } else {
      publishing.Body = []byte(options.Body)
  }

I'm therefore wondering why not make the encoding/decoding in JS either using an existing JS library (in a quick search I found this) or by implementing the msgpack encoding/decoding in its own extension (say xk6-msgpack)

Hello namesake 😄!!

About the implementation is true, understanding what you mean. A concatenation of if-else for each content-type would be horrible.

But as to whether it is the right place to use the ContentType variable, I think so. As far as I know text/plain, application/json, application/msgpack... are data exchange format.

Regarding whether to the encoding in the test itself with js, I think it would not be optimal since anyone who wants to do a test of this type would have to import the library and duplicate code between tests.

javaducky commented 1 year ago

maybe if I used this implementation, would you accept the feature? 😄 => https://github.com/vmihailenco/msgpack

@PabloDelBarrioArnanz Yes, if you were to switch the backing implementation used for the compression, we could accept the PR.

PabloDelBarrioArnanz commented 1 year ago

maybe if I used this implementation, would you accept the feature? 😄 => https://github.com/vmihailenco/msgpack

@PabloDelBarrioArnanz Yes, if you were to switch the backing implementation used for the compression, we could accept the PR.

Hi @javaducky, switched! 😊