565678 - Adapt quickparse.py (ISPDB statistics generator) to DNS MX

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect)

Description

[Ben Bucksch (:BenB)]

TB 3.1, when the primary request fails, also asks for the domain in DNS MX, so we'd potentially get 2 hits for the same attempted configuration, which would screw our statistics about the hit rate (which percentage of requests we could successfully serve).

Bug 565308 is about the client changing the request to let us know the difference.

One way to do this would be to make several hit lists / statistics:
* TB 3.0 clients
* TB 3.1 clients, primary/first domain request
* TB 3.1 clients, secondary / DNS MX domain request (see bug 565308)
* All together, merged

We basically need the whole current output for each of the above subsets of requests.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to remove failures which have successful lookups for their MX domain.

I know this doesn't give us quite the resolution of the data we were hoping for, but I think the numbers are closer, and it seems to work on the files I have tested it with.  (Dropbox/ispdb-logs/all.mx.txt.bz2, and Dropbox/ispdb-logs/20100505.log.bz2)

Later,
Blake.

Comment 2

[Ben Bucksch (:BenB)]

Sorry for being dumb, but could you describe the algo you use, in English, please?

Comment 3

[Ben Bucksch (:BenB)]

I don't exactly know what dict "domains" contains (one entry per domain, with count and success/fail?), but from what I read:
- For all domains that failed, you check whether we have the corresponding MX domain as success, and if so, you remove the original domain.
That's good, but misses another way the stats are screwed currently:
If we have a double miss, meaning neither original domain nor MX domain (which needs to be different from original domain, otherwise TB won't ask again) are in the DB, we now get two misses in the stats instead of one, screwing stats. That needs to be solved somehow, too.

Comment 4

[Ben Bucksch (:BenB)]

Also, your "Effective TLD" check is a bit simple. Check out
import etld # <http://www.stillhq.com/python/etld/etld.py>
# Get the "second level domain", e.g. "mozilla.org" or "bbc.co.uk"
def getSLD(domain):
  try:
    sp = etldService.parse(domain) # returns ("5.4.bbc", "co.uk")
    return re.sub(".*\.", "", sp[0]) + "." + sp[1]
  except Exception:
    return domain

Comment 5

[Ben Bucksch (:BenB)]

> If we have a double miss, meaning neither original domain nor MX domain (which
> needs to be different from original domain, otherwise TB won't ask again) are
> in the DB, we now get two misses in the stats instead of one, screwing stats.
> That needs to be solved somehow, too.

I wonder whether we should add the MX or original in this case. Both are interesting, so ideally we'd have 3 lists (org, mx, combined).

Either way, you can use the same approach as for the other case, i.e. for original domain, you check the MX, and then add the count of original domain to the MX domain and delete original domain, or substract the count of original domain from MX domain.

---

I forgot to say: Thanks for taking a stab at this! The stats as-is are not very useful, esp. the hit rate is all wrong.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch with most of Ben's suggestions.

So, those were a lot of good points.  I'm now stripping out the misses as well, and using the etld code.

I'm not going to do the three lists in this patch, though, since we can always do that later, and this seems like it help us a fair amount as it is, and the three lists would be a little bit harder to fit in.

(I wonder at what point we'll just move to a sqlite database to store all this stuff, so that we can pull out random queries?  ;)

Later,
Blake.

Comment 7

[Ben Bucksch (:BenB)]

> So, those were a lot of good points.  I'm now stripping out the misses
> as well, and using the etld code.

Thanks!

> I'm not going to do the three lists in this patch, though

Yeah, of course, I didn't expect you to.

Comment 8

[Ben Bucksch (:BenB)]

Thanks, bwinton.

I don't see how you remove the "double misses" as described at then end of comment 3, though.
If foobar.com has an MX of mail.foobar.com, we get only one miss in the stats, which is correct. If foobar.com is not in our DB, and it has an MX of hoster.com, which also isn't in our DB, we'll get two misses in our stats, one for foobar.com and one for hoster.com, although it's just one lookup that failed, so this user counts double, which is wrong.

I think your current patch does not solve that, because it's only removing the original domain when the MX is a hit, not when it's a miss.

Just some nits about code readability:
NIT: Please add function descriptions, e.g. for getSLD, e.g. see comment 4. NIT: I think it does good when you document the homepage of third-party modules.
NIT: With "getMX(name)", I was assuming it's a function where I pass in a domain name and get back the MX server name, but it's actually reading all the known MX servers from a file and returning them all as an array, so maybe call it "readMXServersFile" and/or make a comment.
NIT: The comment for the same function or the error output for the commandline argument --mx should also have a line documenting the format of the file, so that you don't have to infer it from code.
NIT: "e = etld.etld()" given that this is a global var and used only in one place, maybe use a longer var name for this.

Comment 9

[Philippe M. Chiasson (:gozer)]

A quick tests with this patched failed for me for 2 reasons.

1. the effective_tld_names.dat file needs to be manually downloaded for the etld.py module to find it
2. all.mx.txt wasn't found, and I can't quite figure out what it's for.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #8)
> Thanks, bwinton.
> 
> I don't see how you remove the "double misses" as described at then end of
> comment 3, though.

"hits = dictify(domains)"

I still call it hits, but it's actually all the domains, not just the cache hits.

> Just some nits about code readability:
> NIT: Please add function descriptions, e.g. for getSLD, e.g. see comment 4.
> NIT: I think it does good when you document the homepage of third-party
> modules.
> NIT: With "getMX(name)", I was assuming it's a function where I pass in a
> domain name and get back the MX server name, but it's actually reading all the
> known MX servers from a file and returning them all as an array, so maybe call
> it "readMXServersFile" and/or make a comment.
> NIT: The comment for the same function or the error output for the commandline
> argument --mx should also have a line documenting the format of the file, so
> that you don't have to infer it from code.
> NIT: "e = etld.etld()" given that this is a global var and used only in one
> place, maybe use a longer var name for this.

(I'll fix the rest of these later, since I've got some higher-priority things on my plate.  But if you wanted to take the patch, and fix it, please feel free.)

(In reply to comment #9)
> A quick tests with this patched failed for me for 2 reasons.
> 
> 1. the effective_tld_names.dat file needs to be manually downloaded for the
> etld.py module to find it

Yeah, the etld.py module mentions that.  And gets the url wrong.  But I didn't think we wanted to add that file to our repo, so I didn't add it.  If we do, let me know, and I'll throw it in.

> 2. all.mx.txt wasn't found, and I can't quite figure out what it's for.

It's the uncompressed copy of "Dropbox/ispdb-logs/all.mx.txt.bz2", which I figured you put there to let me map hostnames to mx lookups.  :)

Later,
Blake.

Comment 11

[Ben Bucksch (:BenB)]

Yes, I think we should add the etld file, to avoid extra work before being able to use it, and surprises when somebody forgets it.

> > 2. all.mx.txt wasn't found, and I can't quite figure out what it's for.

> It's the uncompressed copy of "Dropbox/ispdb-logs/all.mx.txt.bz2", which I
> figured you put there to let me map hostnames to mx lookups.  :)

You really need better documentation, of source code and commandline flags, not even we could figure out what they are for :).

Comment 12

[Philippe M. Chiasson (:gozer)]

Comment on attachment 458339 [details] [diff] [review]
A patch with most of Ben's suggestions.

Fine with me for a first shot at getting more correct stats.

One small nit, however, I generally prefer

#!/usr/bin/env python
to
#!/usr/bin/python

But that's minor.

Comment 13

[Philippe M. Chiasson (:gozer)]

Comment on attachment 458339 [details] [diff] [review]
A patch with most of Ben's suggestions.

Just realized this tool doesn't preform MX lookups itself, so that's a problem. all.mx.txt was just a one-off quick dump of MXes for a couple of days worth of logs.

For this to be accurate, this tool needs to perform DNS mx lookups (and hopefully cache them so as not to make bazillion DNS queries)

Comment 14

[Ben Bucksch (:BenB)]

gozer, getting all MXes for all domains is a long process. The script would run a long time, and gather the same data almost every day. Whether that's a good idea depends on how long the long tail is (how many different domains), and how long a DNS lookup (on fringe domains, not gmail.com) takes from your server.
As for myself, I used the same approach: 2 scripts, one takes the log and gets all MX servers (of course only once per domain), and the second script does the statistics. The first (MX gathering) ran hours for me (on a dedicated server with a good connection), the second (stats processing) takes 10 seconds. So, I think it's a good idea to stay with the all.mx.txt, for pure practical reasons.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with added mx lookup.

(In reply to comment #12)
> Fine with me for a first shot at getting more correct stats.
> 
> One small nit, however, I generally prefer
> 
> #!/usr/bin/env python
> to
> #!/usr/bin/python
> But that's minor.

Yeah, since that's a third-party file, I didn't want to change it.  I've also left some trailing spaces in that file, too.

(In reply to comment #14)
> gozer, getting all MXes for all domains is a long process.

You're not kidding!

./quickparse.py 20100501.log  20.05s user 6.85s system 0% cpu 1:53:59.52 total
./quickparse.py 20100501.log -p 20100501.pickle  3.45s user 0.26s system 68% cpu 5.451 total

I'm dumping the cumulative mx entries into the previous pickle file, which seemed like a reasonable place to put it.

Later,
Blake.

Comment 16

[Ben Bucksch (:BenB)]

Attachment: fix, v4 - fixed usability problems

The previous patch, plus:
- Fixed remaining code problems from comment 8 (bwinton had fixed 2 of them)
- Added more commandline documentation
- Made default save location match the default load location, so that you
  don't have to do anything and it won't repeat the MX lookups.
- Removed ASSIGNED_DOMAINS, because it's counter-productive: even if a domain
  is in progress, I want to know that it's still missing and about its
  ranking. Also, the list of bugs in source code is not maintainable
  (and was terribly outdated) and is already kept at
   <https://wiki.mozilla.org/Thunderbird/TopMissingDomains#Domains>

Comment 17

[Ben Bucksch (:BenB)]

Attachment: fix, v5 - fixed usability problems

Also, rename "hits" - confusing double-use of var name (it's also used below, with completely different and conflicting meaning)

Comment 18

[Ben Bucksch (:BenB)]

Consider this r=BenB on the changes bwinton made :)

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Attachment: v6, hopefully final cleanup.

I've kept most of Ben's changes, and added a few more.
1) Pending was linked to the ASSIGNED_DOMAINS, so I removed that.
2) I feel that if we overwrite the pickle files, we'll lose the ability to re-generate old data, so I reverted that change, but also changed the default previous file to be yesterday's pickle file.
3) I've cleaned up the usage a little, so that it aligns better when you type "quickparse.py -h".
4) I've put the data for the MX hits under the regular hit data, since that seems more conceptually like it.

Thanks,
Blake.

Comment 20

[Ben Bucksch (:BenB)]

reviewed your changes. Looks with with me, with 2 changes:
1. (optional) change
+  print "Reqs IP"
to
+  print "# of requests per single IP"
2. The logfile filename parsing doesn't work, with anything other than your filename convention. I tried
./quickparse.py test.log
and get
Traceback (most recent call last):
  File "./quickparse.py", line 256, in <module>
    sys.exit(main())
  File "./quickparse.py", line 179, in main
    d = date(int(name[0:4]), int(name[4:6]), int(name[6:8]))
ValueError: invalid literal for int() with base 10: 'test'
Same with all.log, access.log etc.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Attachment: v7, with Ben's fixes.

Comment 22

[Ben Bucksch (:BenB)]

Comment on attachment 470477 [details] [diff] [review]
v7, with Ben's fixes.

Thanks!

r=BenB

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Committed as http://viewvc.svn.mozilla.org/vc?view=revision&revision=73346

Should be interesting to see what the new stats are.  :)

Comment 24

[Philippe M. Chiasson (:gozer)]

Made a small followup change to track dns query statistics and handle 2 special kinds of urls : /v1.1/ ones and ?t=n (funnelcake)

http://viewvc.svn.mozilla.org/vc?view=revision&revision=73353

Comment 25

[Ben Bucksch (:BenB)]

So, we don't separate v1.1/gmail.com from gmail.com anymore? That's fantastic! I thought of this after bwinton's commit, but didn't dare to ask for that. This makes a difference, because the percentages of the TB3.1 v1.1 lookups would be off by the percentage of TB3.0 users.

I saw you also made another change.
mxs[name] = possible_mxs[0] 	     mxs[name] = getSLD(possible_mxs[0][1])
That would store only the SLD of the MX server, not the full hostname anymore, correct? We do the getSLD() after the lookup anyway, making this unnecessary, or what am I missing what's the purpose of the change?

> if mx and (mx in domainsDict or (prefix + "/" + mx) in domainsDict):

This is only necessary once, to migrate from the old to new pickle file, no?
We could remove that again, then, esp. given that prefix wouldn't work properly with 2 path dirs instead of 1.

Comment 26

[Blake Winton (:bwinton) (:☕️)]

1) No, we still separate them, so that we can track 3.0 vs. 3.1 ISPDB usage.

2) That's correct, we're only storing the SLD, but what I found was that we didn't do the getSLD after the lookup.  (Check the code, "getSLD" is only called once!)

3) No, I don't think so.  We could still have 3.0 and 3.1 clients hitting the ISPDB, so I think we'll still need this.  And I also believe that having the single path component of "vX.Y" (and query parameters that we're stripping off the end) will probably be good enough for us for quite a while.  :)

Later,
Blake.

Comment 27

[Ben Bucksch (:BenB)]

1) ok, but the 100% must be the set of TB 3.0 and set of TB 3.1 users, not the combination of them. If you have 1000 users of TB 3.1 and 100 users of TB 3.0, and 1% of users use gmx.de, then the current algo will say 0.9% v1.1/gmx.de and 0.1% gmx.de. Given that 0.1% would be too small, it would drop off the list and we don't see it, so it appears that gmx.de as 0.9%, although it has 1%.

2) shouldn't we do getSLD() after the lookup, then, instead of storing just "foo.com" as "MX"? the MX is mailin.foo.com, not foo.com, so the var name is lying now (confusing).

3) whether v1.1 or v1.0 hit is irrelevant wrt to mx, because mx is always mailin.foo.com, never v1.1/mailin.foo.com.

Comment 28

[Blake Winton (:bwinton) (:☕️)]

1) Sure, but I suspect that all the domains will suffer that to approximately the same extent, so everything will still be in the correct order.  :)

2) The MX is mailin.foo.com, but the config Thunderbird will be looking for based on the MX check is foo.com, so it makes more sense (to me) to store that.

3) But Thunderbird 3.1 will be hitting "v1.1/foo.com", and that's what we want to check for.

Later,
Blake.

Comment 29

[Ben Bucksch (:BenB)]

the new list looks off: many formerly top missing domains are no longer in the list, although we don't have a config for them.
If tomorrow is just as odd, can we revert the last change (not the reviewed one, but the one afterwards), so see whether that's the reason?

Comment 30

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to fix the DNS MX overcounting.

Comment 31

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 471102 [details] [diff] [review]
A patch to fix the DNS MX overcounting.

So, it turns out that if a host has itself as its DNS MX entry, we'll incorrectly remove it from the list of domains.

Here's a patch that fixes that.

Comment 32

[Ben Bucksch (:BenB)]

Comment on attachment 471102 [details] [diff] [review]
A patch to fix the DNS MX overcounting.

Unfortunately, this patch regresses this bug. As said on IRC, with this patch, all MX hits are gone from the stats again.
r-

Comment 33

[Blake Winton (:bwinton) (:☕️)]

While that's a nice theory, you're wrong.  ;)

--------
HITS: 418 domains, accounting for 57527 successes, or 43.4% success rate
  MX: 9243 domains, accounting for 26215 hits.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^!!!

MISSES: 23435 domains, accounting for 75116 failures, or 56.6% fail rate
[snip…]

Top 10 hits:
  v1.1/gmail.com (11367 hits, aka 8.6%)
  v1.1/hotmail.com (3445 hits, aka 2.6%)
  gmail.com (2989 hits, aka 2.3%)
  v1.1/yahoo.com (2898 hits, aka 2.2%)
  v1.1/t-online.de (2642 hits, aka 2.0%)
  v1.1/web.de (2531 hits, aka 1.9%)
  v1.1/gmx.de (2377 hits, aka 1.8%)
  v1.1/google.com (2212 hits, aka 1.7%)
  v1.1/free.fr (1157 hits, aka 0.9%)
  v1.1/googlemail.com (985 hits, aka 0.7%)

Top 20 misses:
  v1.1/ocn.ad.jp (535 hits, aka 0.4%)
  v1.1/psmtp.com (469 hits, aka 0.4%)
  v1.1/2iij.net (410 hits, aka 0.3%)
  v1.1/cox.net (384 hits, aka 0.3%)
  v1.1/interia.pl (353 hits, aka 0.3%)
  v1.1/netease.com (336 hits, aka 0.3%)
[snip…]
adding all 20 would boost our HIT rate by 4.4%
Next 50 misses:
[snip…]

# DNS Statistics lookups/cached (hit ratio)
22730/755 (3.2%)
--------

It didn't show any MX hits on your small sample file because you didn't have any MX hits in it.  (The MX for Google Apps domains is googlemail.com or google.com, not gmail.com.  And neither of the MX hosts had hits in the small test file.)

Later,
Blake.

Comment 34

[Ben Bucksch (:BenB)]

> It didn't show any MX hits on your small sample file because you didn't have
> any MX hits in it.  (The MX for Google Apps domains is googlemail.com or
> google.com, not gmail.com.  And neither of the MX hosts had hits in the small
> test file.)

Ah, I see. That's a design error, then, though. I see why and why it's hard to fix (you only look at the log file, not the DB), but it's significant: google.com is the MX domain for Google Apps, but google.com should not have any direct hits, because that's the company domain. You could be lucky that some Google employees hits our database every day, but that won't work for other, signficant domains, e.g. ispgateway.de is an entirely artifical domain with no users, but it's the MX domain for many many customer domains (top1 missing). The stats will be screwed for all these domains.

Comment 35

[Ben Bucksch (:BenB)]

Also, for some reason, the script works before your patch in attachment 471102 [details] [diff] [review], it got at least *this* aspect right.

Comment 36

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #34)
> > It didn't show any MX hits on your small sample file because you didn't have
> > any MX hits in it.  (The MX for Google Apps domains is googlemail.com or
> > google.com, not gmail.com.  And neither of the MX hosts had hits in the small
> > test file.)
> Ah, I see. That's a design error, then, though. I see why and why it's hard to
> fix (you only look at the log file, not the DB), but it's significant:
> google.com is the MX domain for Google Apps, but google.com should not have any
> direct hits, because that's the company domain. You could be lucky that some
> Google employees hits our database every day, but that won't work for other,
> signficant domains, e.g. ispgateway.de is an entirely artifical domain with no
> users, but it's the MX domain for many many customer domains (top1 missing).
> The stats will be screwed for all these domains.

The Google Employee won't, no, but since we're doing an MX lookup, Thunderbird will!

(The whole idea is to remove misses where we have a follow up hit-or-miss to the MX address.)

So it will actually still work!  ;)

Later,
Blake.

Comment 37

[Ben Bucksch (:BenB)]

Ah, right, thanks.

Comment 38

[Ben Bucksch (:BenB)]

Comment on attachment 471102 [details] [diff] [review]
A patch to fix the DNS MX overcounting.

revoking my r-.
can I commit this, given that it has r=gozer?

Comment 39

[Ben Bucksch (:BenB)]

(Or can you?)

Comment 40

[Blake Winton (:bwinton) (:☕️)]

Please do, I won't have time until later today or tomorrow.

Thanks,
Blake.

Comment 41

[Ben Bucksch (:BenB)]

blake: I tried and can't, see bug 593635. So, can you please do it?
gozer, do I have permissions to commit to svn ispdb.momo? If not, could you grant them, please?

Comment 42

[Blake Winton (:bwinton) (:☕️)]

Committed attachment 471102 [details] [diff] [review] as http://viewvc.svn.mozilla.org/vc?view=revision&revision=73850