Conversation
vbudhram
force-pushed
the
fxa-11067
branch
2 times, most recently
from
b30ac39 to
c27b250
Compare
| this.metrics.histogram( | ||
| 'sms.phoneNumberLookup.success.smsPumpingRisk', | ||
| smsPumpingRisk | ||
| ); |
There was a problem hiding this comment.
I don't think I understand this metric.
There was a problem hiding this comment.
I was hoping that we could see the sms pumping risk over time. Histogram seems right?
https://www.twilio.com/docs/glossary/what-is-sms-pumping-fraud
There was a problem hiding this comment.
I think histogram is a right StatsD metric type for it. But I'm trying to think how I'd interpret something like average sms pumping risk over time. Maybe just a lack of imagination on my part.
There was a problem hiding this comment.
A gauge might also be useful, see comment above. That should give us a plot of points over time. Basically I'd just like to sere a line bouncing around. Histrogram works too though. Maybe we are more concerned with the trend of the max values than the averages?
There was a problem hiding this comment.
I say we keep both?
| this.metrics.histogram( | ||
| 'sms.phoneNumberLookup.success.phoneNumberQualityScore', | ||
| phoneNumberQualityScore | ||
| ); |
There was a problem hiding this comment.
I don't understand this either.
There was a problem hiding this comment.
Twillio says this
The Phone Number Quality Score uses a risk-based framework to assess potential risks. Higher scores indicate greater risk, helping identify numbers linked to issues like fraud.
chenba
left a comment
chenba
left a comment
There was a problem hiding this comment.
LGTM although I don't understand the purpose of the two histogram metrics
| } | ||
| const smsPumpingRisk = result?.smsPumpingRisk?.smsPumpingRiskScore; | ||
| if (smsPumpingRisk) { | ||
| this.metrics.histogram( |
There was a problem hiding this comment.
Not sure it matters, but I already added something for this in the service. Search for this.metrics.gauge('sim_pumping_risk', smsPumpingRisk);
I think maybe the service is the better place to do this since we can control the stat for a lookup during phone number confirm, vs a lookup that might happen in the future during a sign in confirmation.
| * the last sim swap period. | ||
| */ | ||
| const simSwapPeriod = result.simSwap?.lastSimSwap?.swappedPeriod; | ||
| if (simSwapPeriod) { |
There was a problem hiding this comment.
Note that simSwapping seems most useful on confirm code for signin, and we don't call phoneNumberLookup at this here at this point in time.
3 participants
Because
This pull request
this.log.infowith statsdIssue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-11128
Closes: https://mozilla-hub.atlassian.net/browse/FXA-11067
Checklist
Other information (Optional)
Dashboards have been started here https://yardstick.mozilla.org/d/eed2dadjju48wc/fxa-sms?orgId=1&from=now-6h&to=now&timezone=browser