Stopped logging warnings for large queue payload

[carbonin]

These messages were not helpful in production mode
and also were being triggered at too low a level.
The original intent was to find where messages with
MBs of data were being saved to the queue.

https://bugzilla.redhat.com/show_bug.cgi?id=1274336

@jrafanie @kbrock

[Fryguy]

What if we just throw it away entirely? @dclarizio ?

We still pay the penalty in dev and test every time we put something on the queue because we still have to call YAML.dump on the args. Is this still a useful feature at all?

[Fryguy]

The original intent was to find where messages with MBs of data were being saved to the queue.

To clarify, the original intent was to find that as well as when we put ActiveRecord objects directly on the queue. It turns out that putting objects on the queue happens to be really large, so it covered both cases, particularly when the size was at 512.

[carbonin]

Ah okay, I spoke to @kbrock about it briefly, but still didn't quite understand why we were doing this all the time. I would rather just remove it if we think that this (putting large objects on the queue) isn't happening any more or have a better way to determine when it does.

[matthewd]

I hesitate to encourage it, but there should surely be some only-moderately-hacky mechanism by which we can grab the already-serialized string, instead of re-doing it ourselves... right?

[Fryguy]

I was thinking that, but I don't think the object has been serialized yet by the AR model. At a high level, we are essentially here in the flow:

q = MiqQueue.new(:args => [some_objects])
# Check the size of the args in q here
q.save!

[Fryguy]

Oh no wait, we do a .create! I'm not sure when that changed, but yeah, if that's there we can surely just use the internal serialization if it exists.

[miq-bot]

Checked commit carbonin@ce1f1a8 with ruby 1.9.3, rubocop 0.34.2, and haml-lint 0.13.0
2 files checked, 2 offenses detected

app/models/miq_queue.rb

• 🔶 - Line 97, Col 34 - Style/NumericLiterals - Separate every 3 digits in the integer portion of a number with underscores(_).

spec/models/miq_queue_spec.rb

• 🔶 - Line 556, Col 86 - Style/NumericLiterals - Separate every 3 digits in the integer portion of a number with underscores(_).

[kbrock]

to be honest, I think this is only significant in production.
@Fryguy do you remember the back story on this warning?

[jrafanie]

Agreed, I don't understand why we need to pay the penalty of yaml dump and potentially logging in every queue put. Even if we have the yaml string, let's figure out what this log message solves. Do we really care about the size or do we really care how long it takes to create or something else?

[Fryguy]

See ☝️ #5393 (comment)

[kbrock]

To clarify, the original intent was to find that as well as when we put ActiveRecord objects directly on the queue. It turns out that putting objects on the queue happens to be really large, so it covered both cases, particularly when the size was at 512.

Thanks, I didn't know about this secondary concern.
That sounds like a development concern and not a production one.

If this is test/development only, should we blow up instead of warn?
Developers will probably not catch the errors in the logs.

[carbonin]

So I found this which looks like it should do what we want in terms of getting rid of YAML.dump, but it doesn't seem to work with the object returned from create!. I'm guessing it's because create will return the object that was saved to the database where new returns just the local object?

I did a quick test with the queue specs and this is what I'm seeing where msg is the object returned from create!:

(byebug) db_msg = MiqQueue.find(msg.id)
#<MiqQueue:0x0055a2bd159108>
(byebug) db_msg == msg
true
(byebug) msg.args_before_type_cast
1
2
3
4
5
(byebug) msg.args_before_type_cast.class
Array
(byebug) db_msg.args_before_type_cast
---
- 1
- 2
- 3
- 4
- 5
(byebug) db_msg.args_before_type_cast.class
String

So it looks like our options are calling back to the database or changing the create! to a new + save to get the size of the serialized args. Thoughts?

[kbrock]

I had played with args_before_type_cast. Heh, I would like to change all new + save! to create!.

Why are we doing this? Is there a way to get our needs met and avoid this check?

1. Is it easier to see the total size of the MiqQueue object and not just the serialized field?
2. Does psych have a safe mode e.g.: Add safe_load ruby/psych#119 - so it will not be able to load other objects. I think blowing up is our best course of action. (would prefer to introduce in 5.6 not 5.5)

[Fryguy]

@dclarizio Need your opinion here.

[kbrock]

Looks like this was added in 08ddbec6f5dd0efcba - the comment doesn't describe why the check was added. So the open question is whether we need it.

Also, whether we want to allow an active record object to go into the queue, or if we want to blow up on the prospect. (e.g. use YAML.load_safe or equivalent)

[Fryguy]

@dclarizio Sorry for the pings...I really confused session size check with queue size checks.

Just to give info on the commit in case we ever come back to this ticket

commit 08ddbec6f5dd0efcbaab2fc125dac360e41439a0
Author: Keenan Brock <kbrock@redhat.com>
Date:   Mon Jan 6 15:51:36 2014 -0500

    Warn if there is too much data passed on the message queue

---

At this point, I'm thinking that if we can get the size from the "should have already been serialized on create" string in the msg, then report it, otherwise throw out the diagnostic altogether. @jrafanie @bdunne @matthewd @kbrock @carbonin what do you think?

[kbrock]

I'm leaning towards throwing this away.

Note, :data is only used in a hand full of places.
grep ':data *=> *Marshal.dump' shows scanning_operations.rb
and scanning_operations_mixin.rb - not sure how much of this is used.

[carbonin]

@Fryguy #5393 (comment) was my attempt to get the serialized args out of msg. I'm hoping that either I missed something in that test or there is another way to do it that doesn't involve reading the record back from the database. Any ideas?

[Fryguy]

another way to do it that doesn't involve reading the record back from the database

Hoping for this. I found msg.instance_variable_get(:@original_raw_attributes)["args"], but man that's ugly. cc @matthewd

[carbonin]

@Fryguy where did we land on this? Is msg.instance_variable_get(:@original_raw_attributes)["args"] really what we want to do?

[carbonin]

Or we could still just get rid of it entirely ...

[kbrock]

I vote removing it

[jrafanie]

I vote removing it

✂️ 🔥 ✂️ 🔥 ✂️ 🔥 ✂️ 🔥 ✂️ 🔥

[carbonin]

Alternative PR here: #5716

[carbonin]

Closing in favor of #5716

[kbrock]

yay