#2643692: SMS User Redesign needs SMS message storage.

Sms_track' purpose is to provide temporary SMS storage. Consider integrating sms_track into core module, and converting storage from plain table to a full entity. This allows us to upgrade \Drupal\sms\Message\SmsMessage to a full entity. Existing code will still work, we would be passing around unsaved stub entities.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpi created an issue. See original summary.

dpi’s picture

Issue summary: View changes
almaudoh’s picture

I had thought of this several times in the past, but it never seemed like a good idea to turn SMS message into entities.

It is not in all use cases that a website may want to keep records of sent messages. Secondly, the entity system is designed for the use cases where user interaction to add / remove fields and change their values is needed via UI. In the case of SmsMessageInterface, it is designed to be a lightweight value object to allow for storing huge volumes if needed (as in the case of bulk messaging sites) - the Entity system would be too heavy and too slow.

So:

  1. Let's consider a way of saving message history that is better than sms_track
  2. But let's do it in a separate module so that those who don't need it can turn it off.
  3. I don't recommend using the entity system either as it is too heavy for this use case.
dpi’s picture

I think you are underestimating how fast the entity system is when you dont have fields or controllers and junk :)

If you dont expose the possibility of fields or UI to the user, then you dont get the slowdown.

The benefit of the entity system in this case is we have the exact same object or derivative interface, you're passing around unsaved stub entity objects. Its just one command away ($obj->save()) from inserting into storage.

Anyways, if you can take a look at this: https://github.com/dpi/courier_sms/blob/8.x-1.x/src/Entity/SmsMessage.php and https://github.com/dpi/courier_sms/blob/8.x-1.x/src/SmsMessageInterface.php

The entity inherits the SMS Framework interface. In this case its SmsMessage (stdclass) -> SmsMessage (entity).

dpi’s picture

Ill do a benchmark tomorrow on actual time and memory creating unsaved courier_sms stub entities

almaudoh’s picture

I think you are underestimating how fast the entity system is when you dont have fields or controllers and junk :)

Ill do a benchmark tomorrow on actual time and memory creating unsaved

I'll be pleasantly surprised to find that D8 entities are as fast as value objects, with all the additional crud (pun not intended) of the entity system. That would perhaps explain why you like the entity system so much. (Yeah, I've checked out Courier and RNG)

dpi’s picture

I'll be pleasantly surprised to find that D8 entities are as fast as value objects

Thats not what I said. Its still a SQL/ORM layer. You weigh the benefits of overhead versus having to develop and maintain a system which Drupal can give you for free.

In any case, how many theoretical SMS message objects are expected to exist during a single request?

Surely < 100 messages.

Even if more are required, that code block is aware of performance issues and should be adding messages to a #2677682: SMS message FIFO dispatch queue. And if required, unloading the entities from memory.

almaudoh’s picture

Surely < 100 messages.

On my site, I sometimes have customized SMS messages sent to up to 5000 recipients, even though the custom module I use splits them into blocks of ~300 recipients.

The second concern is the additional storage space in the database which entities use for field information storage. Each field creates its own table. Of course, if we don't allow custom fields to be added, then that can be mitigated, but I've never tested that though.

dpi’s picture

FileSize
67.04 KB

Again, if you dont give the user an interface to add fields, there is no concern. If you want the field system, you must explicitly code for it.

I dont know how much you are aware of D8 entity system, but a entity type only needs one table: the base field table. This includes the ID field, and any other field defined in basefielddefinitions. My point is: a "sms message" entity uses no more data than the track module currently does:

courier_sms: Entity/SmsMessage.php

entity

Let me be clear. The only overhead is memory and CPU: The Drupal entity system.

On my site, I sometimes have customized SMS messages sent to up to 5000 recipients, even though the custom module I use splits them into blocks of ~300 recipients.

rhetorical So what is the limit, 50000, 1000000, 100000000 messages? If code has no upper limit, then it is its responsibility to implement batching.

I will return with perf benchmarks

dpi’s picture

Issue tags: +beta target
dpi’s picture

Had some thoughts regarding this today, subclass the current SmsMessage class into an entity:

The entity is storage for processing queue, with the following extra data:

  • Boolean flag indicating whether it is processed. This lets messages persist in the database, while giving the system a way of knowing whether a SMS has been processed. (For usage with SMS Track)
  • A flag indicating whether the SMS is to be transmitted through the gateway, or is to be processed as an incoming SMS from the gateway.
dpi’s picture

I've spent some time looking over what would be required to accommodate an integrated #2677682: SMS message FIFO dispatch queue and an SMS archival system (sms_track).

In the interest of moving forward (with #2677682: SMS message FIFO dispatch queue and #2677676: Automated Messages & Sleep), it would be great to catch up on IRC to discuss it.

Turns out I have taken a lot of inspiration from the existing {sms_track} table.

I have created an entity definition here. (branch: dpi/sms-storage-2677684)

Design changes

Gateway annotation: Gateway supports multiple recipients in a single request

Plugin definition should indicate whether the endpoint allows multiple recipients in a single request: When a message is added to the queue, if the gateway does not supports multiple recipients, then it will split the SMS message out into separate queue items.

New per gateway configuration:

  • bool Skip queue. (send in same request.) used for debugging, or low volume site with with no regular cron.
  • int Message retention: incoming: in seconds (set to 0 to delete immediately)
  • int Message retention: outgoing: in seconds (set to 0 to delete immediately)

edit: removed sql table def; d.o formats it incorrectly.

dpi’s picture

FileSize
101.48 KB

schema

adding bitmap of message schema

almaudoh’s picture

Plugin definition should indicate whether the endpoint allows multiple recipients in a single request:

We could make this a max_recipients_per_request property. I have implemented split message sending in sms_gateway and I found different gateways have different limits.

Is the message schema for queue, track or general purpose? What's the purpose of entity target ids? What of the use cases where the sender or recipients are just phone numbers not attached to any entities, e.g. in some bulk SMS applications? I'm available on irc now. Was a bit under the weather for the last few days.

almaudoh’s picture

The other thing to consider is: are we having a 1-to-1 mapping between SMS messages (which could have multiple recipients) and the storage schema above (which makes room for only 1 recipient). The implementation at branch: dpi/sms-storage-2677684 assumes a single recipient.

The implementation of sms_track in HEAD also assumes only a single recipient, but I made some changes in the patch #2542790-38: Implement API for delivery reports for SMS Gateways that corrected that assumption.

dpi’s picture

We could make this a max_recipients_per_request property. I have implemented split message sending in sms_gateway and I found different gateways have different limits.

This sounds great. I think the normal 0=unlimited (aka no known limit) magic number makes sense here.

Is the message schema for queue, track or general purpose?

Technically its for both. As in after a message is processed by the gateway, it will remain in the queue in an archived/expired state. So messages will be deleted if {message retention seconds} == 0 || {now time} > {processed time} + {message retention seconds}.

What's the purpose of entity target ids?

Thats a DER field, they have two columns. Target ID and target entity type. This is used in place of the existing sms_track 'author' and 'recipient' uid fields.

What of the use cases where the sender or recipients are just phone numbers not attached to any entities,

The recipient_entity and sender_entity fields are optional, and are only used for administration UI (similar/same as sms_track 'author' and 'recipient'). The messages are always sent to phone numbers on the 'recipient' field.

If a message is sent to using the phone number service, then we calculate the destination phone number for the sms message. the entity is then only informational usage

are we having a 1-to-1 mapping between SMS messages (which could have multiple recipients) and the storage schema above

actually the 'recipient' field on the entity has unlimited cardinality, which means it creates a new table, and does auto joins.

Thats where the 'max_recipients_per_request' property comes in. If the gateway doesnt support it, then we have to create a message for every recipient. if the gateway does support multiple, then we create one message object with a bunch recipients attached.

dpi’s picture

I forgot to screenshot the other table for #13.

For the entity, there is a total of two tables. The base table, and the recipient table (the field has >1 cardinality)

almaudoh’s picture

if the gateway does support multiple, then we create one message object with a bunch recipients attached.

So for storage purposes (ala sms_track), there needs to be a bunch of reports with corresponding delivery status and message ids matching that bunch of recipients as well. Take note that message ids are unique per message per recipient. The reference field is what I used initially to map to message ids. So in a multi-cardinality recipients field, we have to use something else to identify individual messages.

dpi’s picture

I see.

Of course we could just as well remove the cardinality on recipients, so one entity = one sms + one recipient so it is the same as {sms_track}.

Or it might make sense to have another new table with delivery reports data?

I guess we're back to the question for storage (and/or entity) for delivery reports.

Im deferring to you for domain advice on this one.

dpi’s picture

I think the only things I need to know for this issue is whether to:

  • Revert cardinality of recipients to one.
  • Remove delivery_status field
  • Remove reference field.
almaudoh’s picture

Of course we could just as well remove the cardinality on recipients, so one entity = one sms + one recipient so it is the same as {sms_track}.

Let's not do this.

Or it might make sense to have another new table with delivery reports data?

I think this is a better idea. We can move the reference and delivery status fields to a new table that would hold just information on delivery. While information on sender, message, sendtime, etc. remains in the original table, since this would be the same for all recipients.

dpi’s picture

I hope to return to this shortly. Im currently wrapping up ancillary modules, and a 1.0 for RNG.

dpi’s picture

Assigned: Unassigned » dpi
Status: Active » Needs review

I have returned! It'd be great if you could take a look at this PR.
https://github.com/dpi/smsframework/pull/10
https://github.com/dpi/smsframework/pull/10/files

dpi’s picture

I think we have reached a consensus on the schema. Ill commit this soon so we can refine it further in #2677682: SMS message FIFO dispatch queue. Don't hesitate to provide more feedback.

almaudoh’s picture

Do we have any tests for the Entity. You should post a patch here just to ensure d.o testbots pass.

dpi’s picture

dpi’s picture

Testing patch for 7e7f917

Individual commits added to the PR. No features, just fixing bugs and adding tests.

Details in the commit messages.

dpi’s picture

Testing patch for 5b3d5aa

Again no new features, adding tests for remaining untested methods on smsmessage entity. This should make it 100%

  • dpi committed 0c84241 on 8.x-1.x
    Issue #2677684 by dpi: Added a SMS message storage entity to facilitate...
dpi’s picture

Status: Needs review » Fixed

Commited since there's full coverage. Feel free to provide more feedback.

almaudoh’s picture

Great work!!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

The last submitted patch, 26: sms-storage-2677684.patch, failed testing.

The last submitted patch, 27: sms-storage-2677684-7e7f917.diff, failed testing.

Status: Closed (fixed) » Needs work

The last submitted patch, 28: sms-storage-2677684-5b3d5aa.patch, failed testing.

dpi’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.