Merge lp:~cerberus/nova/nova_notifications into lp:~hudson-openstack/nova/trunk
- nova_notifications
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Josh Kearney |
Approved revision: | 775 |
Merged at revision: | 1086 |
Proposed branch: | lp:~cerberus/nova/nova_notifications |
Merge into: | lp:~hudson-openstack/nova/trunk |
Diff against target: |
347 lines (+306/-0) 7 files modified
nova/flags.py (+3/-0) nova/notifier/__init__.py (+14/-0) nova/notifier/api.py (+83/-0) nova/notifier/log_notifier.py (+34/-0) nova/notifier/no_op_notifier.py (+19/-0) nova/notifier/rabbit_notifier.py (+36/-0) nova/tests/test_notifier.py (+117/-0) |
To merge this branch: | bzr merge lp:~cerberus/nova/nova_notifications |
Related bugs: | |
Related blueprints: |
Notification System
(Medium)
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Josh Kearney (community) | Approve | ||
Brian Waldon (community) | Approve | ||
Jay Pipes (community) | Approve | ||
Rick Harris (community) | Approve | ||
Review via email: mp+58825@code.launchpad.net |
Commit message
Description of the change
Implements a basic mechanism for pushing notifications out to interested parties. The rationale for implementing notifications this way is that the responsibility for them shouldn't fall to Nova. As such, we simply will be pushing messages to a queue where another worker entirely can be written to push messages around to subscribers.
Matt Dietz (cerberus) wrote : | # |
Absolutely Jay. I'll get on that.
Vish Ishaya (vishvananda) wrote : | # |
FWIW I really hate importing things in __init__.py because it leads to strange import chains where things get imported when you didn't expect them to. Is there a reason to put this call here instead of for example in notify/api.py?
Vish
On Apr 22, 2011, at 8:41 AM, Matt Dietz wrote:
> Matt Dietz has proposed merging lp:~cerberus/nova/nova_notifications into lp:nova.
>
> Requested reviews:
> Nova Core (nova-core)
>
> For more details, see:
> https:/
>
> Implements a basic mechanism for pushing notifications out to interested parties. The rationale for implementing notifications this way is that the responsibility for them shouldn't fall to Nova. As such, we simply will be pushing messages to a queue where another worker entirely can be written to push messages around to subscribers.
> --
> https:/
> You are subscribed to branch lp:nova.
> === modified file 'nova/flags.py'
> --- nova/flags.py 2011-04-21 23:01:38 +0000
> +++ nova/flags.py 2011-04-22 15:41:24 +0000
> @@ -369,6 +369,9 @@
> DEFINE_
> 'availability zone of this node')
>
> +DEFINE_
> + 'nova.notifier.
> + 'Default driver for sending notifications')
> DEFINE_
> DEFINE_
> ['hypervisor=
>
> === added directory 'nova/notifier'
> === added file 'nova/notifier/
> --- nova/notifier/
> +++ nova/notifier/
> @@ -0,0 +1,24 @@
> +# Copyright 2011 OpenStack LLC.
> +# All Rights Reserved.
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License"); you may
> +# not use this file except in compliance with the License. You may obtain
> +# a copy of the License at
> +#
> +# http://
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> +# License for the specific language governing permissions and limitations
> +# under the License.
> +
> +from nova import flags
> +from nova import utils
> +
> +FLAGS = flags.FLAGS
> +
> +def notify(event_name, model):
> + """Sends a notification using the specified driver"""
> + driver = utils.import_
> + driver.
>
> === added file 'nova/notifier/
> --- nova/notifier/
> +++ nova/notifier/
> @@ -0,0 +1,19 @@
> +# Copyright 2011 OpenStack LLC.
> +# All Rights Reserved.
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License"); you may
> +# not use this file except in compliance with the License. You may obtain
> +# a copy of the License...
Matt Dietz (cerberus) wrote : | # |
I'm planning to revamp this branch with feedback obtained at the summit. Changes forthcoming...
Jay Pipes (jaypipes) wrote : | # |
Nice improvements, Matt!
Little nits:
145 + """ log notifications using nova's default logging system """
Please remove the spaces before and after enclosing """
107 + raise BadPriorityExce
Need i18n in the string there...
Other than those tiny things, looks excellent.
Cheers,
jay
Brian Waldon (bcwaldon) wrote : | # |
147-154, 179, 217-224: Can we use "message" instead of "payload"? The naming is a little confusing.
Would you mind documenting the notifier drivers a bit better through a base class?
Matt Dietz (cerberus) wrote : | # |
Thanks for the feedback, guys.
Jay: I think I've cleaned up what you suggested.
Brian: I made the message changes
I'm a little confused by the request to implement a base class here. Each notifier only has a single argument "notify" method(but the self reference), and that's all there would be to document amongst them. The functionality after that point is entirely different. Beyond that, I feel like the api.notify method is the real one to watch.
Regarding the idea of base classes, I'm opposed to the practice of implementing interfaces in Python. Given the "duck typing" nature of Python, it seems superfluous. If there was methods or member variables to be shared amongst them, it would of course be different.
I've noticed the practice in Nova is starting to lean the way of implementing interfaces, but the argument that's been made is that it's intended to catch missing functionality from driver classes. However, the right answer, in my opinion, is to ensure you've written tests to that implicit interface.
Brian Waldon (bcwaldon) wrote : | # |
> Brian: I made the message changes
>
> I'm a little confused by the request to implement a base class here. Each
> notifier only has a single argument "notify" method(but the self reference),
> and that's all there would be to document amongst them. The functionality
> after that point is entirely different. Beyond that, I feel like the
> api.notify method is the real one to watch.
>
> Regarding the idea of base classes, I'm opposed to the practice of
> implementing interfaces in Python. Given the "duck typing" nature of Python,
> it seems superfluous. If there was methods or member variables to be shared
> amongst them, it would of course be different.
>
> I've noticed the practice in Nova is starting to lean the way of implementing
> interfaces, but the argument that's been made is that it's intended to catch
> missing functionality from driver classes. However, the right answer, in my
> opinion, is to ensure you've written tests to that implicit interface.
You're definitely right, and the reason I suggested it was more to help with documentation rather than enforcing interfaces. It took me longer than it should have to wrap my head around the system as a whole, and I thought it would help others get up to speed quicker. It really isn't a big deal, just an idea I wanted to run by you. I am ready to approve once the following are addressed:
78: you don't seem to use 'event_name', 'message' should be 'payload'
84: this line doesn't belong
90: 'message' should be 'payload'
92: remove 'payload'
103: 'message' should be 'payload'
114: 'message' should be 'payload'
Rick Harris (rconradharris) wrote : | # |
Looks great.
Not really a nit, just an observation:
> 215 +class RabbitNotifier(
> 216 + """Sends notifications to a specific RabbitMQ server and topic"""
> 217 +
> 218 + def notify(self, message):
It looks like the Notifier /class/ is superflous since we define a single stateless function inside of it and we don't derive the implementations from a base-class.
Should we just make each driver's `notify` proc a function which is called by `api.notify`?
As a side benefit, we get to avoid the cost of instantiating an object for each notification sent (of which there will be a gazillion), as well as the small penalty of later having to gc them.
Matt Dietz (cerberus) wrote : | # |
Good catches Brian. No clue how I messed that up!
Rick: Very good point. Let me rework this a bit...
Matt Dietz (cerberus) wrote : | # |
84: this line doesn't belong
Rewrote the comment to better explain what I meant by including that.
Think I covered the rest. Also made changes based on Rick's suggestion
Brian Waldon (bcwaldon) wrote : | # |
> Think I covered the rest. Also made changes based on Rick's suggestion
78, 88, 118: I still feel like 'message' on these lines should be 'payload'. It's confusing to give the notify function a 'message' which is encapsulated as the 'payload' in a new type of 'message' structure.
- 773. By Matt Dietz
-
Conceded :-D
Rick Harris (rconradharris) wrote : | # |
Brian:
Looks like patch has been updated to use 'payload', if it looks good, minding throwing an 'Approve' on there?
Brian Waldon (bcwaldon) wrote : | # |
Thanks for the work, Matt. Sorry to be so picky.
Chris Behrens (cbehrens) wrote : | # |
Looks pretty awesome.
Minor nit that I notice in the tests:
test_send_
notify_called = False
def mock_notify(cls, *args):
notify_called = True
Similar thing in the other tests. self.mock_cast and self.test_topic.
Josh Kearney (jk0) wrote : | # |
Per HACKING, I would stick an extra newline after #59, #145 and #208.
- 774. By Matt Dietz
-
Merge from trunk
- 775. By Matt Dietz
-
Spacing changes
Matt Dietz (cerberus) wrote : | # |
Chris: that actually doesn't work because of the way Python handles scoping in assignments. Your way would create variables scope local to the mock method only, so there would be no way of checking them later in the test.
Josh: Spacing changes made.
Rick Harris (rconradharris) wrote : | # |
> Your way would create variables scope local to the mock method only, so there would be no way of checking them later in the test.
A bit of a hack, but, one way around this is to create a locally-scoped dict which the mocked function can mutate.
monitored = dict(notified=
def fake_notify():
monitored[
self.assertTrue
(* Neutral on whether it should be used in this case *)
Preview Diff
1 | === modified file 'nova/flags.py' |
2 | --- nova/flags.py 2011-04-29 22:06:18 +0000 |
3 | +++ nova/flags.py 2011-05-18 17:56:56 +0000 |
4 | @@ -369,6 +369,9 @@ |
5 | DEFINE_string('node_availability_zone', 'nova', |
6 | 'availability zone of this node') |
7 | |
8 | +DEFINE_string('notification_driver', |
9 | + 'nova.notifier.no_op_notifier', |
10 | + 'Default driver for sending notifications') |
11 | DEFINE_list('memcached_servers', None, |
12 | 'Memcached servers or None for in process cache.') |
13 | |
14 | |
15 | === added directory 'nova/notifier' |
16 | === added file 'nova/notifier/__init__.py' |
17 | --- nova/notifier/__init__.py 1970-01-01 00:00:00 +0000 |
18 | +++ nova/notifier/__init__.py 2011-05-18 17:56:56 +0000 |
19 | @@ -0,0 +1,14 @@ |
20 | +# Copyright 2011 OpenStack LLC. |
21 | +# All Rights Reserved. |
22 | +# |
23 | +# Licensed under the Apache License, Version 2.0 (the "License"); you may |
24 | +# not use this file except in compliance with the License. You may obtain |
25 | +# a copy of the License at |
26 | +# |
27 | +# http://www.apache.org/licenses/LICENSE-2.0 |
28 | +# |
29 | +# Unless required by applicable law or agreed to in writing, software |
30 | +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT |
31 | +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the |
32 | +# License for the specific language governing permissions and limitations |
33 | +# under the License. |
34 | |
35 | === added file 'nova/notifier/api.py' |
36 | --- nova/notifier/api.py 1970-01-01 00:00:00 +0000 |
37 | +++ nova/notifier/api.py 2011-05-18 17:56:56 +0000 |
38 | @@ -0,0 +1,83 @@ |
39 | +# Copyright 2011 OpenStack LLC. |
40 | +# All Rights Reserved. |
41 | +# |
42 | +# Licensed under the Apache License, Version 2.0 (the "License"); you may |
43 | +# not use this file except in compliance with the License. You may obtain |
44 | +# a copy of the License at |
45 | +# |
46 | +# http://www.apache.org/licenses/LICENSE-2.0 |
47 | +# |
48 | +# Unless required by applicable law or agreed to in writing, software |
49 | +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT |
50 | +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the |
51 | +# License for the specific language governing permissions and limitations |
52 | +# under the License.import datetime |
53 | + |
54 | +import datetime |
55 | +import uuid |
56 | + |
57 | +from nova import flags |
58 | +from nova import utils |
59 | + |
60 | + |
61 | +FLAGS = flags.FLAGS |
62 | + |
63 | +flags.DEFINE_string('default_notification_level', 'INFO', |
64 | + 'Default notification level for outgoing notifications') |
65 | + |
66 | +WARN = 'WARN' |
67 | +INFO = 'INFO' |
68 | +ERROR = 'ERROR' |
69 | +CRITICAL = 'CRITICAL' |
70 | +DEBUG = 'DEBUG' |
71 | + |
72 | +log_levels = (DEBUG, WARN, INFO, ERROR, CRITICAL) |
73 | + |
74 | + |
75 | +class BadPriorityException(Exception): |
76 | + pass |
77 | + |
78 | + |
79 | +def notify(publisher_id, event_type, priority, payload): |
80 | + """ |
81 | + Sends a notification using the specified driver |
82 | + |
83 | + Notify parameters: |
84 | + |
85 | + publisher_id - the source worker_type.host of the message |
86 | + event_type - the literal type of event (ex. Instance Creation) |
87 | + priority - patterned after the enumeration of Python logging levels in |
88 | + the set (DEBUG, WARN, INFO, ERROR, CRITICAL) |
89 | + payload - A python dictionary of attributes |
90 | + |
91 | + Outgoing message format includes the above parameters, and appends the |
92 | + following: |
93 | + |
94 | + message_id - a UUID representing the id for this notification |
95 | + timestamp - the GMT timestamp the notification was sent at |
96 | + |
97 | + The composite message will be constructed as a dictionary of the above |
98 | + attributes, which will then be sent via the transport mechanism defined |
99 | + by the driver. |
100 | + |
101 | + Message example: |
102 | + |
103 | + {'message_id': str(uuid.uuid4()), |
104 | + 'publisher_id': 'compute.host1', |
105 | + 'timestamp': datetime.datetime.utcnow(), |
106 | + 'priority': 'WARN', |
107 | + 'event_type': 'compute.create_instance', |
108 | + 'payload': {'instance_id': 12, ... }} |
109 | + |
110 | + """ |
111 | + if priority not in log_levels: |
112 | + raise BadPriorityException( |
113 | + _('%s not in valid priorities' % priority)) |
114 | + driver = utils.import_object(FLAGS.notification_driver) |
115 | + msg = dict(message_id=str(uuid.uuid4()), |
116 | + publisher_id=publisher_id, |
117 | + event_type=event_type, |
118 | + priority=priority, |
119 | + payload=payload, |
120 | + timestamp=str(datetime.datetime.utcnow())) |
121 | + driver.notify(msg) |
122 | |
123 | === added file 'nova/notifier/log_notifier.py' |
124 | --- nova/notifier/log_notifier.py 1970-01-01 00:00:00 +0000 |
125 | +++ nova/notifier/log_notifier.py 2011-05-18 17:56:56 +0000 |
126 | @@ -0,0 +1,34 @@ |
127 | +# Copyright 2011 OpenStack LLC. |
128 | +# All Rights Reserved. |
129 | +# |
130 | +# Licensed under the Apache License, Version 2.0 (the "License"); you may |
131 | +# not use this file except in compliance with the License. You may obtain |
132 | +# a copy of the License at |
133 | +# |
134 | +# http://www.apache.org/licenses/LICENSE-2.0 |
135 | +# |
136 | +# Unless required by applicable law or agreed to in writing, software |
137 | +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT |
138 | +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the |
139 | +# License for the specific language governing permissions and limitations |
140 | +# under the License. |
141 | + |
142 | +import json |
143 | + |
144 | +from nova import flags |
145 | +from nova import log as logging |
146 | + |
147 | + |
148 | +FLAGS = flags.FLAGS |
149 | + |
150 | + |
151 | +def notify(message): |
152 | + """Notifies the recipient of the desired event given the model. |
153 | + Log notifications using nova's default logging system""" |
154 | + |
155 | + priority = message.get('priority', |
156 | + FLAGS.default_notification_level) |
157 | + priority = priority.lower() |
158 | + logger = logging.getLogger( |
159 | + 'nova.notification.%s' % message['event_type']) |
160 | + getattr(logger, priority)(json.dumps(message)) |
161 | |
162 | === added file 'nova/notifier/no_op_notifier.py' |
163 | --- nova/notifier/no_op_notifier.py 1970-01-01 00:00:00 +0000 |
164 | +++ nova/notifier/no_op_notifier.py 2011-05-18 17:56:56 +0000 |
165 | @@ -0,0 +1,19 @@ |
166 | +# Copyright 2011 OpenStack LLC. |
167 | +# All Rights Reserved. |
168 | +# |
169 | +# Licensed under the Apache License, Version 2.0 (the "License"); you may |
170 | +# not use this file except in compliance with the License. You may obtain |
171 | +# a copy of the License at |
172 | +# |
173 | +# http://www.apache.org/licenses/LICENSE-2.0 |
174 | +# |
175 | +# Unless required by applicable law or agreed to in writing, software |
176 | +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT |
177 | +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the |
178 | +# License for the specific language governing permissions and limitations |
179 | +# under the License. |
180 | + |
181 | + |
182 | +def notify(message): |
183 | + """Notifies the recipient of the desired event given the model""" |
184 | + pass |
185 | |
186 | === added file 'nova/notifier/rabbit_notifier.py' |
187 | --- nova/notifier/rabbit_notifier.py 1970-01-01 00:00:00 +0000 |
188 | +++ nova/notifier/rabbit_notifier.py 2011-05-18 17:56:56 +0000 |
189 | @@ -0,0 +1,36 @@ |
190 | +# Copyright 2011 OpenStack LLC. |
191 | +# All Rights Reserved. |
192 | +# |
193 | +# Licensed under the Apache License, Version 2.0 (the "License"); you may |
194 | +# not use this file except in compliance with the License. You may obtain |
195 | +# a copy of the License at |
196 | +# |
197 | +# http://www.apache.org/licenses/LICENSE-2.0 |
198 | +# |
199 | +# Unless required by applicable law or agreed to in writing, software |
200 | +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT |
201 | +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the |
202 | +# License for the specific language governing permissions and limitations |
203 | +# under the License. |
204 | + |
205 | + |
206 | +import nova.context |
207 | + |
208 | +from nova import flags |
209 | +from nova import rpc |
210 | + |
211 | + |
212 | +FLAGS = flags.FLAGS |
213 | + |
214 | +flags.DEFINE_string('notification_topic', 'notifications', |
215 | + 'RabbitMQ topic used for Nova notifications') |
216 | + |
217 | + |
218 | +def notify(message): |
219 | + """Sends a notification to the RabbitMQ""" |
220 | + context = nova.context.get_admin_context() |
221 | + priority = message.get('priority', |
222 | + FLAGS.default_notification_level) |
223 | + priority = priority.lower() |
224 | + topic = '%s.%s' % (FLAGS.notification_topic, priority) |
225 | + rpc.cast(context, topic, message) |
226 | |
227 | === added file 'nova/tests/test_notifier.py' |
228 | --- nova/tests/test_notifier.py 1970-01-01 00:00:00 +0000 |
229 | +++ nova/tests/test_notifier.py 2011-05-18 17:56:56 +0000 |
230 | @@ -0,0 +1,117 @@ |
231 | +# Copyright 2011 OpenStack LLC. |
232 | +# All Rights Reserved. |
233 | +# |
234 | +# Licensed under the Apache License, Version 2.0 (the "License"); you may |
235 | +# not use this file except in compliance with the License. You may obtain |
236 | +# a copy of the License at |
237 | +# |
238 | +# http://www.apache.org/licenses/LICENSE-2.0 |
239 | +# |
240 | +# Unless required by applicable law or agreed to in writing, software |
241 | +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT |
242 | +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the |
243 | +# License for the specific language governing permissions and limitations |
244 | +# under the License. |
245 | + |
246 | +import nova |
247 | + |
248 | +from nova import context |
249 | +from nova import flags |
250 | +from nova import rpc |
251 | +import nova.notifier.api |
252 | +from nova.notifier.api import notify |
253 | +from nova.notifier import no_op_notifier |
254 | +from nova.notifier import rabbit_notifier |
255 | +from nova import test |
256 | + |
257 | +import stubout |
258 | + |
259 | + |
260 | +class NotifierTestCase(test.TestCase): |
261 | + """Test case for notifications""" |
262 | + def setUp(self): |
263 | + super(NotifierTestCase, self).setUp() |
264 | + self.stubs = stubout.StubOutForTesting() |
265 | + |
266 | + def tearDown(self): |
267 | + self.stubs.UnsetAll() |
268 | + super(NotifierTestCase, self).tearDown() |
269 | + |
270 | + def test_send_notification(self): |
271 | + self.notify_called = False |
272 | + |
273 | + def mock_notify(cls, *args): |
274 | + self.notify_called = True |
275 | + |
276 | + self.stubs.Set(nova.notifier.no_op_notifier, 'notify', |
277 | + mock_notify) |
278 | + |
279 | + class Mock(object): |
280 | + pass |
281 | + notify('publisher_id', 'event_type', |
282 | + nova.notifier.api.WARN, dict(a=3)) |
283 | + self.assertEqual(self.notify_called, True) |
284 | + |
285 | + def test_verify_message_format(self): |
286 | + """A test to ensure changing the message format is prohibitively |
287 | + annoying""" |
288 | + |
289 | + def message_assert(message): |
290 | + fields = [('publisher_id', 'publisher_id'), |
291 | + ('event_type', 'event_type'), |
292 | + ('priority', 'WARN'), |
293 | + ('payload', dict(a=3))] |
294 | + for k, v in fields: |
295 | + self.assertEqual(message[k], v) |
296 | + self.assertTrue(len(message['message_id']) > 0) |
297 | + self.assertTrue(len(message['timestamp']) > 0) |
298 | + |
299 | + self.stubs.Set(nova.notifier.no_op_notifier, 'notify', |
300 | + message_assert) |
301 | + notify('publisher_id', 'event_type', |
302 | + nova.notifier.api.WARN, dict(a=3)) |
303 | + |
304 | + def test_send_rabbit_notification(self): |
305 | + self.stubs.Set(nova.flags.FLAGS, 'notification_driver', |
306 | + 'nova.notifier.rabbit_notifier') |
307 | + self.mock_cast = False |
308 | + |
309 | + def mock_cast(cls, *args): |
310 | + self.mock_cast = True |
311 | + |
312 | + class Mock(object): |
313 | + pass |
314 | + |
315 | + self.stubs.Set(nova.rpc, 'cast', mock_cast) |
316 | + notify('publisher_id', 'event_type', |
317 | + nova.notifier.api.WARN, dict(a=3)) |
318 | + |
319 | + self.assertEqual(self.mock_cast, True) |
320 | + |
321 | + def test_invalid_priority(self): |
322 | + def mock_cast(cls, *args): |
323 | + pass |
324 | + |
325 | + class Mock(object): |
326 | + pass |
327 | + |
328 | + self.stubs.Set(nova.rpc, 'cast', mock_cast) |
329 | + self.assertRaises(nova.notifier.api.BadPriorityException, |
330 | + notify, 'publisher_id', |
331 | + 'event_type', 'not a priority', dict(a=3)) |
332 | + |
333 | + def test_rabbit_priority_queue(self): |
334 | + self.stubs.Set(nova.flags.FLAGS, 'notification_driver', |
335 | + 'nova.notifier.rabbit_notifier') |
336 | + self.stubs.Set(nova.flags.FLAGS, 'notification_topic', |
337 | + 'testnotify') |
338 | + |
339 | + self.test_topic = None |
340 | + |
341 | + def mock_cast(context, topic, msg): |
342 | + self.test_topic = topic |
343 | + |
344 | + self.stubs.Set(nova.rpc, 'cast', mock_cast) |
345 | + notify('publisher_id', |
346 | + 'event_type', 'DEBUG', dict(a=3)) |
347 | + self.assertEqual(self.test_topic, 'testnotify.debug') |
Looks reasonable as a starting point for a watch system. One request, though, could you elaborate in the docstrings what the "model" is expected to be? Mapping? Object? A string?