RabbitMQ support for Statements #869
Conversation
|
Hi @ryanrolds, thanks for the pull request. I'm not sure that this is something we'd merge into core, especially in V1. However, I will discuss this tomorrow with @andrewhickey and @ht2. |
|
Any reason for that assessment? This seem exactly the right place to put this feature. Trying to put it up/down stream just makes it harder and would require more infrastructure to provide high availability. If there are things I need to clean up or add please let me know, I will take care of it. |
|
Hi @ryanrolds, thanks for this. As Ryan mentioned we'll review tomorrow but I could see how this would have good community use. We just need to plan it as part of the release cycle. We'll put heads together and get back to you over the next couple of days |
|
Thank you. Have a good week. |
|
Hey @ryanrolds, we've discussed this today. We would like to merge it, however, there are few things that need to be done before we merge. First thing on that list is that it needs some tests and the tests need to pass in Travis. You might find the following resources useful: |
|
@ryansmith94 Will do. I will try to have the changes done before the end of next week. |
|
Update, I'm on a few high priority tasks at work and this is on the back burner. I will be getting to it, but it will probably be a week. This is an important bit for my next major project. |
|
This seems like a really odd feature to integrate into Learning Locker. To me, it seems like this implements the inverse of what the natural relationship should be between these two technologies. Why should Learning Locker be responsible for knowing how to send statements to a RabbitMQ exchange? This seems outside the scope of an LRS, no? Given that RabbitMQ is a message broker, it seems more appropriate for clients to send xAPI statements to a RabbitMQ exchange, which then forwards the statement to interest parties; one of which would be a worker responsible for forwarding all statements to Learning Locker. Is that not how most people do it? On a related note, there have already been several requests (#778 , #779; probably others) for adding webhooks into Learning Locker. This would be a much more generic solution to the same problem, allowing easier integration of other technologies - not just RabbitMQ. So it really surprises me that the devs would be interesting in integrating something like this, instead of moving forward with webhooks. |
|
The design I envisioned is Client -> Load Balancer -> Array of LLs -> Mongo cluster. That keeps the number of failure points down and provides fault tolerance. It's critical that the experience data make in to the datastore. Adding additional potential failure points between the client and the datastore is not desirable. A message broker is needed when additional processing or event handling is needed and we want that work to performed outside of services along in the critical path. The cleanest solution would for the Mongo cluster to queue the messages, but as far as I'm aware Mongo does not support a write/update stream. So, we have to go one layer toward the client and place the queuing system in the application layer. While not deal, it's a perfectly fine and common location to put the queuing. Rather then adding web workers to Learning Locker, IMO it would optimal to use RabbitMQ (or some other message broker/queue) to forward events to workers that perform the required web hook requests. This keeps the load of the web hooks outside of LL (and the critical path), which would be a much more burdensome then the basic AMPQ client required by RabbitMQ. |
|
Having client -> balancer -> ll array does make sense for simple lrs-only solutions. However, beyond that, requiring the LRS to know how to forward statements to a very specific third-party technology seems overly restrictive and far beyond the responsibilities of an LRS. That seems more like a technology stack concern. Note that RabbitMQ also has a form of persistence (durable queues) and could provide the same level of fault tolerance using a simple: client -> rmq broker setup. From here, you can easily tack on any other technology, including load balanced Learning Locker clusters; that is what RabbitMQ was designed to do. Dispatching via webhooks would not be any more or less expensive than dispatching via RabbitMQ. A webhook is just a fancy name for a web-based callback mechanism; the server itself only handles dispatching, the actual event processing/work is handled by the client that provided the callback. Using webhooks, clients get to choose which technology they want to use for event processing; in most cases this would be the web-stack they already have setup. In contrast, this pull request would require clients to install and configure a RabbitMQ server, find an AMQP library compatible with their language of choice, and then figure out how to manage AMQP workers on their system. Doesn't that seem overly opinionated and restrictive for a Learning Record Store? |
|
An array of LL instances does make sense when the load is too great for a single VM and in situations were high availability is important and VMs in different availability zones are used. IMO, if client a performs a request I want that 200 response to be a confirmation that it made it to the Mongo cluster, not that it made it in to the message queue. I understand what web hooks are and I'm telling you the overhead of HTTP is higher then that of AMQP. (http://stackoverflow.com/questions/16838416/service-oriented-architecture-amqp-or-http https://www.quora.com/What-makes-HTTP-better-than-AMQP-for-a-queueing-solution). What do you suggest an LL based web hook implementation do then the other side of the outbound request is down or errors, something that the instance/cluster maintainers may not have any say in? Having the web hook implementation (especially when retry handling is required) outside of LL is desirable. With my suggestion the MQ is under the control of the people maintaining the LL instance/cluster and any issues with the MQ can be immediately addressed. Your argument is that you're used to HTTP and that's what you want, great! This PR isn't about adding web hook support to LL. Go advocate in those other PRs and stop shitting up this PR. |
|
The model you're suggesting is: client -> HTTP to AQMP -> Rabbit MQ -> AMPQ to HTTP -> LL -> Mongo cluster. Do you see how silly that is? This PR is not trying to solve losing experience data due to LL downtime (which is less of an issue with HA setup - array of LL instances behind an LB). It's trying to support event/data streams to additional services in a multi-service architectures without adding unnecessary latency, failure points, or complexity. Again, if you want to advocate for web hooks go do that in those other PRs. |
|
You really don't have to take it so personally and result to vulgar language. Just as you want to see further improvements to Learning Locker, so do I and I'm sure many other people. As a developer that interacts with Learning Locker on a daily basis, my only intention here is to first understand your situation and why you think that this particular technology should be integrated with Learning Locker. I am also already familiar with existing systems similar to what you are describing; RabbitMQ, load balancers, LL, Mongo clusters, etc. Though as noted the stack order for RabbitMQ -> LL is reversed compared to what you are proposing. Seeing those systems already in place and working quite well, I'm merely trying to understand what critical issues exist that would necessitate a merging of these two technologies when they can already happily co-exist as-is. Also, as someone who has advocated for webhooks in the past, I am also curious as to the HT2 teams interest in this particular integration when they've previously postponed similar requests for an imo more generic/common solution. I don't think it makes sense, given that Learning Locker is a web-based service, to require the installation, configuration, and management of RabbitMQ just to enable a pub/sub mechanism within Learning Locker. Especially when that mechanism could be more easily implemented and managed by clients using the web-based (http) architecture Learning Locker itself is already built upon; the webhook alternative would add zero technological overhead for LL clients. |
|
You're not making valid points. The feature is optional and it's a simple and straight forward configuration. It's not uncommon for "web-based service" to include things like MQs. There are no rules saying that MQs can't be driven by HTTP, in fact it's a common use case. You're drawing lines between types of services based on your knowledge, not any kind of industry taxonomy. This feature is simply adding support for an additional MQ, this kind of functionality is already present in LL (https://github.com/LearningLocker/learninglocker/blob/develop/app/config/queue.php) provided by Laravel, it may not be wired up but it's there. Do you object to the existence of support of Redis or SQS? Why do you object for the addition of RabbitMQ (a more sophisticated MQ) support? You're in here not because this is a bad feature or PR. You're in here because you're butt hurt that they haven't added web hooks. Again, stop wasting my time and stop shitting up this PR. |
|
You are a very, very sad person. |
Conflicts: composer.lock
|
Good morning guys, Thank you both for your rather... spirited contributions to this PR. To be completely frank, there are good points raised by both of you, and I certainly do not want to get dragged into any more heated exchanges! Regarding this PR, I see no reason this can't be a core part of this repository. As Ryan says, what he has added is entirely possible using Redis without any modifications (short of the event trigger statement). This update moves only to provide additional functionality for those that would require it. Saying that, I completely sympathise with your view Jamaar that webhooks are a potentially useful way of implementing similar solutions. They both serve a purpose and different use cases will call on both these strategies. For example, for an application that simply wants to be forwarded statements as they come in, then web hooks could be a great way of handling that. However, in other situations you may want to design an analytical application that can simply connect to a message system database without having to worry about exposing ports, worrying about latency and storing the incoming messages. In that scenario, a message system is ideal and allows developers to get on building the exciting bits they want, without also worrying about designing tech stacks that are already available and work extremely efficiently. @jamarr81; If web hooks are very much a priority for you, then I would ask you look to begin work on such features in a fork and submit them in via a PR. We would be delighted to consider that as part of the core project. I would be interested in seeing how you would handle the extra load this would place on the LRS, specifically if you are planning to transport all the statement data via the hook (rather than just the IDs as specified in this PR). That, of course, is for discussion in its own areas. The previous PR that have tried to deal with this problem have simply lacked sufficient tests, especially given the hefty UI amendments that it involved. In that PR there was no opt-in, whereas this PR's system is only going to affect those that choose to enable it via the configs. @ryanrolds: What you have here looks good, but I would still like to see some simple tests around the firing of the One thing I would note is that both of these problems are something we are looking to resolve in V2 of Learning Locker. Obviously that isn't a great help to you now, but they are on our agendas. The last thing I would ask, is that we please try and keep things cordial. I understand it can be frustrating to get your own points across when you have a great idea and others don't seem to be taking them on board, but I will have no time for personal remarks or foul language. I love the community we are building around Learning Locker, whether it be the contributors on Github, the real time collaboration on Gitter or my favourite, face to face conversations at xAPI camps and meetups. |
|
@ht2 (James), thank you for your time and seeing the value of this PR and the potential benefits it affords. It's been a busy few weeks. I can say that as of yesterday afternoon writing tests and getting them in this PR is at the top of my work queue and I'm actively working on it. I don't have an ETA as I haven't worked with PHPUnit, I'm more of a gtest and Mocha person. While it creates additional work for me, I will add a message queue factor to this PR. Thank you for the suggestion, my goal is to not lock LL in to a specific MQ implementation. I hope to have the additional changes and the tests in the next few business days. |
|
Addressed the issues. Added some additional details (configurable) about each statement in the published message. |
| 'stored' => $model['stored']->sec | ||
| ]; | ||
|
|
||
| return array_intersect_key($result, $this->modelKeys); |
There was a problem hiding this comment.
I think we need to allow the use of nested keys in here, like statement.id for example. To be honest, I think it might be ok for now to just add statement.id as a key above in the result.
app/config/messagequeue.php
Outdated
| 'verify_peer'=> true | ||
| ] | ||
| ], | ||
| 'model_keys' => ['id', 'lrs_id', 'client_id', 'statement', 'stored'] |
There was a problem hiding this comment.
statement.id should be included in this default config not the entire statement. I know for some of our instances of LL that including the entire statement would be far too much data.
There was a problem hiding this comment.
Add a second array that tracks what's included from the statement? My only concern is there should be a way to ask for all statement first order keys, otherwise we run in to future proofing issues.
Conflicts: app/locker/repository/Statement/EloquentInserter.php
|
Added the filters and merged master. |
There was a problem hiding this comment.
Hey @ryanrolds - this is looking really good. Could I ask you expand on your test a bit to check that the "includes" work on fields within the statement. I can see that the exclude on single level fields is tested here (https://github.com/LearningLocker/learninglocker/pull/869/files#diff-fb6cbdd314fa995c31d98285ed715805R77) but there is no test for that nested include/exclude fields.
I'd also like to strip down the default include to be:
'message_includes' => ['id', 'lrs_id', 'client_id', 'statement.id', 'stored']
As we discussed, I really don't want to promote the notion that sending these entire objects around is the default way. Happy that it can be configured differently, but these defaults tend to be what 99.99% of people end up doing.
|
Tests added and adjusted the defaults. |
ryasmi
dismissed
ht2’s stale review
Requested changes appear to have been made.
|
Awaiting your review @ht2 |
app/locker/queues/RabbitMQ.php
Outdated
| } | ||
|
|
||
| // Publish to topic exchange | ||
| $key = 'statements'; |
There was a problem hiding this comment.
Could this be part of the rabbitmq config?
|
@ryanrolds thanks for all your hard work here! I know there was a lot of back and forth, so thanks for persisting! 😄 🎉 |
Motive
We need to feed statements to a set of workers so that we can send near real-time notifications or perform other tasks based on a user's interaction with our courses.
Result
Added a RabbitMQ config file, an event listener, and an event emitter. Any Statements being stored in MongoDB are also sent to a RabbitMQ exchange (expects a preconfigured topic exchange, name/id is configurable).
Additional information
SSL connections to RabbitMQ are supported.