Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deliverability Solution #153

Merged

Conversation

bchrobot
Copy link
Member

@bchrobot bchrobot commented May 13, 2019

The approach for evaluating domain health in cron job is:

Every 10 minutes:

  • fetch all messages with links from the last 4 hours
  • group by domain
  • separate out sensor domains and work horse domains
  • calculate deliverability across all sensor domains (this account for one sensor domain going bad)
  • calculate deliverability for each work horse domain
  • mark a work horse domain as unhealthy if its deliverability is ??? relative to the average sensor domain deliverability

@bchrobot bchrobot requested a review from ben-pr-p May 13, 2019 17:52
@bchrobot
Copy link
Member Author

Tested fc7dc36 with logging:

16:30:45 server.1                 |  Replaceing domains for message Hello Leonor Rau. This is Benjamin. Link: https://bs4pres.us/foobar-baz
16:30:45 server.1                 |  Fetched short link domains [ 'brnie2020.us',
16:30:45 server.1                 |    'bs4pres.us',
16:30:45 server.1                 |    'bchrobot.us',
16:30:45 server.1                 |    'bchrobot.me',
16:30:45 server.1                 |    'bchrobot.ly' ]
16:30:45 server.1                 |  doesContainShortLink true
16:30:45 server.1                 |  Fetching domainRaw
16:30:45 server.1                 |  Fetched domainRaw Result {
16:30:45 server.1                 |    command: 'UPDATE',
16:30:45 server.1                 |    rowCount: 1,
16:30:45 server.1                 |    oid: NaN,
16:30:45 server.1                 |    rows: [ anonymous { domain: 'brnie2020.us' } ],
16:30:45 server.1                 |    fields:
16:30:45 server.1                 |     [ Field {
16:30:45 server.1                 |         name: 'domain',
16:30:45 server.1                 |         tableID: 102452,
16:30:45 server.1                 |         columnID: 3,
16:30:45 server.1                 |         dataTypeID: 25,
16:30:45 server.1                 |         dataTypeSize: -1,
16:30:45 server.1                 |         dataTypeModifier: -1,
16:30:45 server.1                 |         format: 'text' } ],
16:30:45 server.1                 |    _parsers: [ [Function: noParse] ],
16:30:45 server.1                 |    RowCtor: [Function: anonymous],
16:30:45 server.1                 |    rowAsArray: false,
16:30:45 server.1                 |    _getTypeParser: [Function: bound ] }
16:30:45 server.1                 |  Extracted targetDomain brnie2020.us
16:30:45 server.1                 |  Replaced all domains. Final text: Hello Leonor Rau. This is Benjamin. Link: https://brnie2020.us/foobar-baz

@bchrobot bchrobot marked this pull request as ready for review May 15, 2019 16:31

await Promise.all(Object.keys(domainDeliverability).map(async domain => {
const deliverability = domainDeliverability[domain];
if (deliverability < sensorDeliverability - 0.30) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is completely made up

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move it to a DELIVERABILITY_DIFF_THRESHOLD variable at the top

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could mark everything is still good if the sensor domains are performing really poorly, so we want a floor (< max(FLOOR, sensoryDeliverability - DELIVERABILITY_DIFF_THRESHOLD)

We also probably want a MIN_SAMPLE_SIZE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the sensor-domain strategy works (assumption), maybe we should attribute poor sensor-domain performance to external factors and mark the workhorse-domain differently than when the sensor-domain is performing well. markDomainUnhealthy vs. markNetworkUnhealthy? Although it may not be practically useful if the response is to wait in both cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should DELIVERABILITY_DIFF_THRESHOLD be based on the sample size? Is that sound?

@bchrobot
Copy link
Member Author

This is being merged and deployed now, but without the cron job enabled. For now, staff will enable/disable domains manually based on a Metabase query for domain deliverability percentage. We will revisit automated health checks.

@bchrobot bchrobot merged commit 27966cb into politics-rewired/deploy May 20, 2019
@bchrobot bchrobot deleted the politics-rewired/deliverability-solution branch May 20, 2019 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants