Content type (de)serialization #152

errm · 2015-08-20T12:36:19Z

Ok so this is my second bash at one of the ideas mooted in #126.

This allows ContentTypes to be configured like so:

Sneakers::ContentType.register(
  content_type: 'application/json',
  deserializer: ->(payload) { JSON.parse(payload) },
  serializer: ->(payload) { JSON.dump(payload) },
)

Then when messages arrive with the given content type metadata set the payload should be deserialized neatly.

Its also possible to add content_type: 'whatever/thing' to the worker options, in order that every message will be deserialized using the given type, irrespective of what the message metadata purports.

The publishing methods now also use the serializers when content_type is present in the options.

If nothing is registered everything just falls back to a noop pair of (de)serializers that just pass through the message unchanged, so this shouldn't break anyones existing applications.

justinmills · 2015-08-20T13:05:10Z

lib/sneakers/worker.rb

@@ -52,10 +53,11 @@ def do_work(delivery_info, metadata, msg, handler)
          metrics.increment("work.#{self.class.name}.started")
          Timeout.timeout(@timeout_after, Timeout::Error) do
            metrics.timing("work.#{self.class.name}.time") do
+              deserialized_msg = ContentType[@content_type || metadata[:content_type]].deserialize(msg)


There's no docs on this method indicating metadata will always be present and it's not private so you might need to handle the metadata == nil case.

Separately I think there are cases where you would want to prefer the worker's content type over the message's and cases for the inverse. Might be worth refactoring that bit of logic into a method that respects a new preference set on the worker indicating which order to prefer.

justinmills · 2015-08-20T13:07:36Z

This is a nifty idea. We've done something similar but with base worker classes that delegate from #work to a new method on the subclass. This is far more elegant.

jondot · 2015-08-20T14:44:22Z

I agree! it looks elegant and in the spirit of the existing api.
Is there a way for users to opt out of the overhead, perhaps short-circuit it, in case they do some non-standard serde? and in case they already have code that does this which they want to keep until they migrate off to this solution?

errm · 2015-08-20T16:04:06Z

Ok made some changes that it short circuits if no content types are registered. This halves the overhead, but at the cost of some elegance.

Before

require 'benchmark'
require 'sneakers/content_type'

msg = "foo"
itr = 10_000_000

def dummy(something)
  something
end

Benchmark.bmbm do |x|
  x.report("content_type lookup:") { itr.times { dummy(Sneakers::ContentType[nil].serialize(msg)) } }
  x.report("no lookup:") { itr.times { dummy(msg) } }
end

Rehearsal --------------------------------------------------------
content_type lookup:   3.550000   0.000000   3.550000 (  3.542536)
no lookup:             0.990000   0.000000   0.990000 (  0.998273)
----------------------------------------------- total: 4.540000sec

                           user     system      total        real
content_type lookup:   3.610000   0.000000   3.610000 (  3.606535)
no lookup:             0.990000   0.000000   0.990000 (  0.989097)

After

require 'benchmark'
require 'sneakers/content_type'

msg = "foo"
itr = 10_000_000

def dummy(something)
  something
end

Benchmark.bmbm do |x|
  x.report("content_type lookup:") { itr.times { dummy(Sneakers::ContentType.serialize(msg, nil)) } }
  x.report("no lookup:") { itr.times { dummy(msg) } }
end

Rehearsal --------------------------------------------------------
content_type lookup:   1.740000   0.000000   1.740000 (  1.741147)
no lookup:             0.990000   0.000000   0.990000 (  0.988361)
----------------------------------------------- total: 2.730000sec

                           user     system      total        real
content_type lookup:   1.760000   0.000000   1.760000 (  1.766063)
no lookup:             0.980000   0.000000   0.980000 (  0.986241)

errm · 2015-08-20T16:21:02Z

Its a difference of 0.185μs, do we care about performance that much? We are basically optimising away 2 method dispatches ...

errm · 2015-08-20T16:34:27Z

re @justin-yesware
I agree there is one more use case where you might want to usually take the content/type from the metadata, but then have a default on the worker. But on the whole i think it is not going to be a widely used option.

I prefer the simplicity of:

set it on the worker (or globally) explicitly and have the behaviour set in stone.
--or--
Have workers that can magically work with messages with a number of serialisation formats, but the producers had better set the content_type to something that you expect if you want to do that.

In my experience I do want to spend some time making sure that my producers are formatting messages in a somewhat sane way, and wrapping some tests around that stuff rather than having to have the worker work to hard to work out what it is getting.

Having said that I don't feel overly strongly about the issue and am happy to add some option to flip the priority if we can think of a good name for it.

justinmills · 2015-08-20T18:15:57Z

I don't feel strongly about it, just wanted to call it out. What you've done is the hard part so it should be easy to extend it if that use case ever comes up.

errm · 2015-08-21T18:41:49Z

@jondot, @justin-yesware how do we feel about this now? Is this good enough or would you like me to explore some more ways of making it simpler to optionally include the feature...

If you guys are happy, shall I squash my commits?

justinmills · 2015-08-22T22:21:06Z

Overall this looks great. Looking forward to picking this up next time we're writing json/thrift messages. One question about the default values in the #register method. What's the use case for a nil content type. Also, is the expectation to allow a nil serializer/deserializer if you plan to only use the publish/subscribe feature of sneakers? Might be worth some documentation indicating that. These are just nits though 😄.

errm · 2015-08-23T04:48:53Z

TBH re .register I would have preferred to have content_type as a required argument, unfortunately it's not a supported thing in ruby 2.0 to do def foo(bar:) that didn't come in until 2.1.

It might be useful to add a guard condition like:

raise ArgumentError, 'missing keyword: content_type' unless content_type

I am not sure if we should do some validation on the serializer/deserializer:

Clearly if neither is supplied the registration is not useful, but would it be better to fail hard, or log a warning in this case.
In the case where only one is supplied, but then we attempt to use the other, what should we do? Fail or try and fall back to a passthough?
Both of the above scenarios would be avoided if both serializer and deserializer were required arguments, perhaps that is better, and not too much of an overhead for someone to setup?

justinmills · 2015-08-24T12:34:08Z

Those are all very good points. I asked because the method declaration has content_type: nil explicitly in it, not so much because it could be nil. Re-reading the change, it's not clear that that default value for content_type is required (all callers seem to pass in a content_type value), so maybe it can be dropped? I realize that's not making it required, but at least Ruby would prevent you from calling #register without any args.

gabrieljoelc · 2015-10-06T23:53:38Z

Maybe it could include a couple shortcuts for some common ones:

ContentType.register_json
ContentType.register_xml

errm · 2016-03-31T16:33:26Z

Hey @jondot I noticed this PR has been rotting for a while, how do you feel about it? I am happy to do some more work on it or give up ?

errm · 2016-06-06T07:04:28Z

ping . . . .

jondot · 2016-06-06T07:36:07Z

Hi @errm thanks for the wakeup call, I find that Github email notifications suck.. I must solve that some day.
I'll start going over issues and PRs now

jondot · 2016-06-06T08:00:44Z

@errm I'm still very happy with this work!, let's get it merged. I'm working on a new documentation website, instead of the Wiki. Just so I know how to document this, what are the implicit things that happen here? For example, if I don't specify content_type at all - does it use json by default? (looks like it..?)

errm · 2016-06-08T08:30:33Z

@jondot Sounds great, give me a ping if you wan't me to help/read anything documentation wise.

As far as what should happen implicitly - nothing. It's covered here: spec/sneakers/content_type_spec.rb:33. Basically internally the hash of ContentTypes is defaulted to a passthrough content type that has ->(payload) { payload } for the serializer and deserializer.

If you want to make use of this, it is required to explicitly register a content type, thus:

Sneakers::ContentType.register(
  content_type: 'application/json',
  deserializer: ->(payload) { JSON.parse(payload) },
  serializer: ->(payload) { JSON.dump(payload) },
)

Are you still supporting ruby 2.0.x if not we can loose a lot of the cruft from lib/sneakers/content_type.rb:4-14 I think 2.0.x is EOL now so it should be fine.

registered yet.

I was not really happy that trying to short circuit and save a few method calls when the feature is not in use only to impact on the feature itself. This makes the whole thing a little more efficient by switching to attr_reader and imposes a constant cost whichever execution path is followed.

Short circuiting with a guard condition like this does not seem to impose any significant cost to the happy path.

errm · 2017-01-26T18:27:23Z

ping ... rebased this again

michaelklishin · 2017-01-26T19:17:38Z

@jondot @gabrieljoelc do we have a consensus that this feature is worth including (worth the overhead, primarily)? I'm somewhat on the fence.

michaelklishin · 2017-01-26T19:17:48Z

@errm thank you for keeping this up-to-date.

gabrieljoelc · 2017-02-07T06:20:17Z

@michaelklishin do you have some specifics about your concerns?

michaelklishin · 2017-02-07T08:22:25Z

@gabrieljoelc I haven't noticed that the difference is for 10M iterations. So I guess this is good to go if we are willing to have this feature.

@gabrieljoelc @jondot should I merge?

gabrieljoelc · 2017-02-10T03:51:57Z

@michaelklishin are you thinking that this should be in a separate extension library?

michaelklishin · 2017-02-10T08:31:20Z

@gabrieljoelc this can be part of Sneakers proper.

michaelklishin · 2017-03-06T21:36:48Z

@gabrieljoelc @jondot so, should we merge this?

gabrieljoelc · 2017-03-07T07:30:33Z

I approved it.

…

On Mon, Mar 6, 2017, 2:36 PM Michael Klishin ***@***.***> wrote: @gabrieljoelc <https://github.com/gabrieljoelc> @jondot <https://github.com/jondot> so, should we merge this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#152 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJBiQrxgwONahvgtva2bIZPyQtsc9ebks5rjHxwgaJpZM4FvF67> .

michaelklishin · 2017-04-12T17:32:53Z

This is finally in.

justinmills reviewed Aug 20, 2015
View reviewed changes

errm force-pushed the content_type_serialization branch from 72f94d7 to e2a4cb6 Compare January 26, 2017 17:35

errm added 6 commits January 26, 2017 17:52

Adds Sneakers::ContentType

a432d11

Hold a flag to see if any content-types have been

995ed42

registered yet.

More efficient short circuit . . .

3c8a1f1

Short circuiting with a guard condition like this does not seem to impose any significant cost to the happy path.

Make content_type 'Required'

43fde3f

Do some basic checks when the ContentType is registered

a3b73be

errm force-pushed the content_type_serialization branch from e2a4cb6 to a3b73be Compare January 26, 2017 17:55

gabrieljoelc approved these changes Mar 7, 2017

View reviewed changes

michaelklishin merged commit 56cc08e into jondot:master Apr 12, 2017

errm deleted the content_type_serialization branch June 14, 2017 09:41

gabrieljoelc mentioned this pull request Aug 24, 2017

Bump to 2.6.0 #311

Merged

ansoncat mentioned this pull request Oct 19, 2020

Support content-encoding in worker/publisher #449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content type (de)serialization #152

Content type (de)serialization #152

errm commented Aug 20, 2015

justinmills Aug 20, 2015

justinmills commented Aug 20, 2015

jondot commented Aug 20, 2015

errm commented Aug 20, 2015

errm commented Aug 20, 2015

errm commented Aug 20, 2015

justinmills commented Aug 20, 2015

errm commented Aug 21, 2015

justinmills commented Aug 22, 2015

errm commented Aug 23, 2015

justinmills commented Aug 24, 2015

gabrieljoelc commented Oct 6, 2015

errm commented Mar 31, 2016

errm commented Jun 6, 2016

jondot commented Jun 6, 2016

jondot commented Jun 6, 2016

errm commented Jun 8, 2016

errm commented Jan 26, 2017

michaelklishin commented Jan 26, 2017

michaelklishin commented Jan 26, 2017

gabrieljoelc commented Feb 7, 2017

michaelklishin commented Feb 7, 2017

gabrieljoelc commented Feb 10, 2017

michaelklishin commented Feb 10, 2017

michaelklishin commented Mar 6, 2017

gabrieljoelc commented Mar 7, 2017 via email

michaelklishin commented Apr 12, 2017

Content type (de)serialization #152

Content type (de)serialization #152

Conversation

errm commented Aug 20, 2015

justinmills Aug 20, 2015

Choose a reason for hiding this comment

justinmills commented Aug 20, 2015

jondot commented Aug 20, 2015

errm commented Aug 20, 2015

Before

After

errm commented Aug 20, 2015

errm commented Aug 20, 2015

justinmills commented Aug 20, 2015

errm commented Aug 21, 2015

justinmills commented Aug 22, 2015

errm commented Aug 23, 2015

justinmills commented Aug 24, 2015

gabrieljoelc commented Oct 6, 2015

errm commented Mar 31, 2016

errm commented Jun 6, 2016

jondot commented Jun 6, 2016

jondot commented Jun 6, 2016

errm commented Jun 8, 2016

errm commented Jan 26, 2017

michaelklishin commented Jan 26, 2017

michaelklishin commented Jan 26, 2017

gabrieljoelc commented Feb 7, 2017

michaelklishin commented Feb 7, 2017

gabrieljoelc commented Feb 10, 2017

michaelklishin commented Feb 10, 2017

michaelklishin commented Mar 6, 2017

gabrieljoelc commented Mar 7, 2017 via email

michaelklishin commented Apr 12, 2017