Page MenuHomePhabricator

Add SMS support
ClosedPublic

Authored by btrahan on May 1 2014, 6:24 PM.
Tags
None
Referenced Files
F14095295: D8930.diff
Mon, Nov 25, 8:19 PM
F14094041: D8930.diff
Mon, Nov 25, 1:56 PM
Unknown Object (File)
Wed, Nov 20, 6:32 PM
Unknown Object (File)
Wed, Nov 20, 5:04 PM
Unknown Object (File)
Wed, Nov 20, 5:04 PM
Unknown Object (File)
Wed, Nov 20, 5:04 PM
Unknown Object (File)
Wed, Nov 20, 5:04 PM
Unknown Object (File)
Wed, Nov 20, 5:04 PM
Tokens
"Doubloon" token, awarded by epriestley.

Details

Reviewers
epriestley
chad
Maniphest Tasks
T920: Provide SMS Support
Commits
Restricted Diffusion Commit
rPe96c363eefe7: Add SMS support
Summary

Provides a working SMS implementation with support for Twilio.

This version doesn't really retry if we get any gruff at all. Future versions should retry.

Test Plan

used bin/sms to send messages and look at them.

Diff Detail

Repository
rP Phabricator
Branch
T920
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/infrastructure/sms/worker/PhabricatorSMSSendWorker.php:43XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 315
Build 315: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

btrahan retitled this revision from to Work in Progress - SMS support.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
externals/twilio-php
1 ↗(On Diff #21194)

I guess I need to just do a file copy here rather than a git clone?

src/infrastructure/sms/adapter/PhabricatorSMSImplementationAdapter.php
53–64

this could easily just for a foreach ($to_numbers) and kill the DemultiplexWorker

src/infrastructure/sms/adapter/PhabricatorSMSImplementationTwilioAdapter.php
16

the object we get back now will have an ID and it *may* indicate some success / failure. most likely it will say "queued" though.

Twilio will post back to a configurable URI with the ID and status update when the status has updated.

src/infrastructure/sms/storage/PhabricatorSMS.php
4–18

wildly unused at the moment but theoretically will

  • have some ID column that Twilio et al will populate
  • will be used for retrying until things work
src/infrastructure/sms/worker/PhabricatorSMSWorker.php
7–10

I wasn't sure what would actually be useful here

what, if any other providers are there than Twilio for this sort of thing?

Twilio is the only one I'm actively aware of / recognize the brand name of. We might want a "drop in a hole" adapter for testing, but I don't think we need to support anything else out of the gate.

Looks like there are a ton of them in general, though. I don't recognize any of these names other than Twilio (and the major carriers) offhand:

http://blog.mashape.com/post/56272188360/list-of-50-sms-apis

should this be a full blown app? (as opposed to some code in the infrastructure folder)

I don't think so -- I'd guess that (like MetaMTA) all the info ends up being too sensitive to put on the web. I'd probably shove the code in infrastructure/ or metamta/ and then make a CLI bin/sms to provide a list, a send-test, etc.

any big changes needed here based on what you see?

Nope, this looks great to me.

would be good to know what other providers do before making this work (e.g. what can I assume the message ID will look like?)

I'd probably just guess / go with your gut, it should be pretty easy to update this table later. We can GC it after 90 days or something like we do with mail, so it won't be too big.

externals/twilio-php
1 ↗(On Diff #21194)

Yeah, just copy the files in.

src/infrastructure/sms/adapter/PhabricatorSMSImplementationAdapter.php
44–46

(We should probably move away from this pattern -- in MetaMTA too -- and just use "throw on failure, don't throw on success". The MetaMTA code would be simpler for it, and everything else everywhere does that.)

53–64

Yeah maybe a good call for v1. This is just an INSERT and I think it will be rare that we need to send a whole lot of SMSes at once.

src/infrastructure/sms/adapter/PhabricatorSMSImplementationTwilioAdapter.php
16

Oh bleh. Ah well, not a big issue.

src/infrastructure/sms/storage/PhabricatorSMS.php
8–10

Maybe we need a WE_THINK_THIS_IS_SENT_BUT_IT_IS_OUT_OF_OUR_HANDS option too for Twilio at least.

src/infrastructure/sms/worker/PhabricatorSMSDemultiplexWorker.php
18–26

I would expect this to (at least eventually) do something like:

  • create an SMS object
  • save it
  • queue a SendWorker with the object ID (i.e., not pass the data directly to the worker)?

Also I think it's better (maybe) not to encode the adapter in the task (does MetaMTA do this?). If Twilio goes down, it seems good to be able to switch to Threelio or whatever and have all your queued messages send automatically?

src/infrastructure/sms/worker/PhabricatorSMSWorker.php
7–10

Yeah -- the number and text both seem too sensitive to disclose. Maybe just leave it for now.

Thank you sir.

btrahan edited edge metadata.
NOTE: see Daemons section below if you have time. That's the big juicy stuff you could help me with.

CAVEAT - this is in the "almost done for review" phase, which is to say I am testing it and it isn't working and I am confused but figure it can't take long now...! I need to cook dinner though and am not sure how much evening time I will get, so serving up now in case you get to take a gander.

Lots and lots of updates - addressed all the feedback and made other changes...

Daemons

We have an interesting potential problem in that Twilio, etc. actually retries on its own for awhile, so unless they can truly say they failed re-sending on our side we will end up sending duplicates. Further, Twilio, etc. claim to fail rarely and that most errors are configuration or rate limiting errors, so retrying naively in the doWork method is going to not work or spam SMS to folks. As such, re-jiggered this a bit to have a poll inside the "doWork" method, and before checkin I think I need to retry if and only if the external provider has said "we definitely failed." thoughts?

any pro-tips for debugging daemons and workers? finding it hard to surface phlog, echo, etc and will probably write to a temp file if there's no better way to surface info.

in doWork, if everything went okay, you just return? what if things went badly but not "fail permanently" badly? is there a way to sleep for a bit then hop in the top of the doWork method without that counting as a try?

(answers to these questions would let me implement the best retry strategy possible as cleanly as possible.)

As such, re-jiggered this a bit to have a poll inside the "doWork" method, and before checkin I think I need to retry if and only if the external provider has said "we definitely failed." thoughts?

Ideally, we should distinguish between "this is failed forever" (e.g., bad "to" number) and "this is appropriate to retry" (e.g. rate limit).

Thinking about this, for v0, I kind of thing we should maybe just never retry. A lot of mail we really want to try hard to deliver, but the only SMSes we're going to send right now are multi-factor SMSes, which are no good if they take more than a minute or so to show up. Generally, SMSes are likely to be time sensitive, and delivering them much later is probably annoying at best (your phone blows up) and confusing at worst (you get a bunch of SMS codes which aren't valid anymore, or alerts for things that already happened).

So the logic would look like:

  • If we get a good request, record the queued/sending status but just "return;" from the task and call it a success, assuming it will do the right thing. Presumably, it is very rare and/or nonexstent that Twilio queues a message and then fails to deliver it for reasons where retries would be appropriate.
  • If we get a bad request, no matter what, just throw PermanentFailure so we don't retry.

Then based on our experience in the real world we can see if that's good enough or not? My guess is that it will be like 98% good, and then adding retries for specific errors (e.g., rate limit) will get us to like 99%+ good.

any pro-tips for debugging daemons and workers? finding it hard to surface phlog, echo, etc and will probably write to a temp file if there's no better way to surface info.

Stop all the daemons, then run bin/phd debug taskmaster to guarantee that process gets to process everything. Then all the log(), etc., will show up in your console.

in doWork, if everything went okay, you just return? what if things went badly but not "fail permanently" badly? is there a way to sleep for a bit then hop in the top of the doWork method without that counting as a try?

You can throw new PhabricatorWorkerYieldException(15); to mean (approximately): don't increase the failure count, and retry this task in about 15-ish seconds. (It may take longer than 15 seconds before the task gets retried, but that should be OK here I think).

externals/twilio-php
1 ↗(On Diff #21386)

(Probably on your list, but don't subproject this since users can never remember to git update --init --subprojects --blah blah.)

src/applications/config/option/PhabricatorSMSConfigOptions.php
27

(These defaults are whacky -- just null them?)

29–40

Call setLocked() on these to prevent a rogue administrator from redirecting outbound SMSes to a service they control.

41–45

Call setHidden() on this to prevent a rogue administrator from reading the secret.

src/infrastructure/sms/storage/PhabricatorSMS.php
33

(Do we really need this?)

src/infrastructure/sms/storage/PhabricatorSMSDAO.php
4–12

Maybe just dump this on the MetaMTA database? Seems related-ish and sort of silly to make an extra database for.

Also, we use "multiplex" elsewhere but I think your usage is actually correct and they should all be "demultiplex". I think I just never looked it up, I'll maybe fix that at some point.

btrahan retitled this revision from Work in Progress - SMS support to Add SMS support.May 8 2014, 9:31 PM
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan edited edge metadata.

small update to show-outbound workflow

src/infrastructure/sms/worker/PhabricatorSMSSendWorker.php
43

this TODO here is a bit of a misnomer in that the work won't happen inside this if().

The return; will bite the dust though. =D

72

as is, this is typically STATUS_SENT_UNCONFIRMED and as is we'll never get to STATUS_SENT. I think this is fine though since this isn't really exposed anywhere and leaves us where we need to be for future iteration. I can do whatever here though.

epriestley edited edge metadata.

Yeah, this seems quite reasonable to me. I think it's perfectly fine to leave 'em in "probably sent, not totally sure" for now.

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
122

Can we get rid of this?

This revision is now accepted and ready to land.May 9 2014, 1:56 AM
btrahan edited edge metadata.

remove unnecessary database creation

btrahan updated this revision to Diff 21449.

Closed by commit rPe96c363eefe7 (authored by @btrahan).