857107 - [l10nstats][bb2mbdb] Support showing compare-locales from elasticsearch storage in addition to local log files, add migration command

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Axel Hecht [:Pike]]

+++ This bug was initially created as a clone of Bug #741957 +++

In bug 741957, we decided to store our elmo buildbot logs in ES.

I've got work in progress, but that bug turned rather lengthy, with detours etc, so cloning that bug, and taking the actual implementation work here.

Comment 2

[Axel Hecht [:Pike]]

Attachment: create index, and convert existing logs to ES via django commands. uses pyelasticsearch

Erik, can I pester you for another round of reviews?

I addressed the comments from bug 741957 comment 15.

Open question: I guess I need some form of throttle in the uploadlogs command to not completely bog down the ES cluster? Given the amount of data, and how the load balancing works, I might be pounding the poor thing for hours as fast as I possibly can. (I wish python had a decent way to check if there's a key pressed without requiring it, then I could gracefully stop, too)

Probably want to redo latestlog to be actually written for each block, and not at last, to be safe. Saving that for later, I guess there's enough to say about the patch as is.

Comment 3

[Axel Hecht [:Pike]]

Attachment: show logs from both ES and file

Patch to show the logs, trying both ES and local files. Requesting review from Erik on the ES parts, I guess that much of this patch is probably better for Peter to review. In particular the piece where I start showing the "html" logs at all, which we store in the db, and continue to store there. They're rare and only created for buildbot-internal errors during the builds, thus that's fine.

Comment 4

[Erik Rose [:erik][:erikrose]]

> pyelasticsearch==0.3

Might as well upgrade to 0.4.1. It fixes a bunch of bugs and is completely backward compatible. 0.5 will be out soon.

> #ES_LOG_INDEX = elmo-logs'

Oops, missed an apostrophe--not that it matters.

>  'Save buildbot logs from disk into ElasticSearch'

In createlogindex.py, I think this was an errant copy-and-paste from uploadlogs.py.

> do_delete = options.get('delete', False)

I guess you decided not to put the default value in the make_option call as I suggested?

> for master, in Master.objects.values_list('name'):

I still vote for `flat=True`, as it's harder to misread and constructs less useless data structure behind the scenes. [Ed: you did this in your other file, so I think you just missed a spot here.]

> print 'Backed up %s' % ll

print → self.stdout.write

> if 'replica' in options:

I think you mean "replicas"; you used the plural everywhere else.

> try:
>    es.create_index(settings.ES_LOG_INDEX, indexargs)
> except Exception, e:

The only likely way for this to fail is with an `IndexAlreadyExistsError` (new in 0.4), so you might want to catch only that one. The way you have it isn't going to hurt you, but it's inconsistent with how you handled the delete_index call. There, you catch only the expected error nicely but let any other exception bubble up. There isn't much of a practical impact here, just something to think about.

> latestlog = (Log.objects

latest_log_id would be a better name, since this isn't actually a full Log object.

> if not logs:

When you do this, the QuerySet gets evaluated, which means all the rows get fetched from the DB.

1. This is expensive in IO and RAM. (I'm assuming this is quite a bit of data.) `if not logs.exists()` would be more efficient, as it would avoid actually fetching all the row data.

2. What's the point of the early exit at all? It seems like it wouldn't take much logic to avoid it. Though, if you switch to exists(), it's cheap.

> pages = Paginator(logs, self.chunksize)
> for pagenum in pages.page_range:

How many logs are you planning to insert in one command invocation? If it's more than a million or two, you might notice indexing start to slow down. If this happens, it's because MySQL is continually recomputing the large filter query and then taking increasingly late offsets of it each time around the loop. It still has to compute the whole thing (up to the offset) each time, though. You can sometimes short-circuit this by keeping track of what the last pkey was and adding a pk__gt constraint to the filter the next time around the loop. However, MySQL can't use more than one index at a time, so that can start to hurt your performance in other ways, depending on your other filters. A compound index is the answer if that happens.

It's worth at least a TODO, because it's hard to figure this out.

> docs += [

To avoid constructing a big secondary list, we should use docs.extend() here and a generator expression within, rather than a list comprehension.

> for chunk, block in izip(
>         generateLogFromFile(master.name, filename, None),
>         count())]

Rather than izip(whatever, count()), try enumerate(whatever). It's shorter and more idiomatic. And it's a builtin.


What's "log_finished" for?

Why make `chunksize`, `es`, and `index` instance vars? Why not pass them right into handleMaster? It's more evident what handleMaster() needs, and it'll aid testing.

> rv = self.es.bulk_index(self.index, 'chunk', docs)

bulk_index() will default to pulling the doc ID out of the 'id' key of the document, but you don't include such a key. ES will make up IDs for you, and they will be long, ugly, unmemorable, and hard to correlate with the pkeys from the DB. Is this what you want?

That's all I've got for attachment 732376 [details] [diff] [review].

Comment 5

[Erik Rose [:erik][:erikrose]]

Comment on attachment 732376 [details] [diff] [review]
create index, and convert existing logs to ES via django commands. uses pyelasticsearch

Review of attachment 732376 [details] [diff] [review]:
-----------------------------------------------------------------

See https://bugzilla.mozilla.org/show_bug.cgi?id=857107#c4. Sorry, just now figuring out how to do reviews through Bugzilla.

Comment 6

[Erik Rose [:erik][:erikrose]]

Comment on attachment 732377 [details] [diff] [review]
show logs from both ES and file

Review of attachment 732377 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple of things here. All in all, your use of pyelasticsearch looks quite good.

::: apps/tinder/tests.py
@@ +312,5 @@
>              f.write(SAMPLE_BUILD_LOG_PAYLOAD)
>  
> +        try:
> +            lm = settings.LOG_MOUNTS
> +            del settings.LOG_MOUNTS

Django typically gets cranky when its settings are mutated except by its @override_settings decorator. You'd do much better to use it, as you'd be sure to put things back properly even if your test explodes before doing the restoration.

::: apps/tinder/views.py
@@ +708,5 @@
> +    body = {
> +        "sort": ["block"],
> +        "filter": filter_
> +    }
> +    filter_['term'] = {'filename': filename, 'master': master}

Does this work? I've never seen anyone try to cram multiple fields into a single term filter before. I would have expected something like this:

ands = [{"term": {"filename": filename},
        {"term": {"master": master}]

if channels:
    ands.append({'terms': {'channel_id': channels}})

query = {
    "sort": ["block"],
    "filter": {
        "and": {
            "filters": [
                ands
            ]
        }
    }
}

@@ +717,5 @@
> +        ]
> +        filter_['and'] = ands
> +    es = pyelasticsearch.ElasticSearch(settings.ES_LOG_HOST)
> +    try:
> +        hits = es.search(query=body, index=settings.ES_LOG_INDEX)

You don't have to (and probably shouldn't) say "query=" here, as it's a required arg.

@@ +718,5 @@
> +        filter_['and'] = ands
> +    es = pyelasticsearch.ElasticSearch(settings.ES_LOG_HOST)
> +    try:
> +        hits = es.search(query=body, index=settings.ES_LOG_INDEX)
> +    except:

No bare excepts allowed. :-)

@@ +729,5 @@
> +    def _iter(_hits):
> +        for hit in _hits:
> +            yield {'channel': hit['_source']['channel_id'],
> +                   'data': hit['_source']['content']}
> +    return _iter(hits)

You could also do this as a one-liner with a generator expression. In any case, I'm not sure why you would break the iterator off as an inner function and return its result rather than just making the outer function a generator.

Comment 7

[Peter Bengtsson [:peterbe]]

Comment on attachment 732377 [details] [diff] [review]
show logs from both ES and file

Review of attachment 732377 [details] [diff] [review]:
-----------------------------------------------------------------

Tempted to r- based on the simple fact that pyelasticsearch is missing from vendor-local. Was that intentional?

Also, what's missing is some sort of guidance in settings/local.py-dist about how to set up the ES config details. Is that also intentional?

::: apps/tinder/templates/tinder/showbuild.html
@@ +20,4 @@
>  <dt>{{ step.text|join:" " }}</dt>
>  <dd>
>    {% for log in step.logs.all %}
> + <a href="{% url 'tinder.views.showlog' build.builder.master.name,build.builder.name,build.buildnumber,step.name,log.name%}">{{log.name}}</a>

Why does this need to be one massive unspaced line?

::: apps/tinder/views.py
@@ +771,4 @@
>                      offset = 0
>                  else:
>                      offset += cnt
> +                if channels is None or channel in channels:

Can you make that `if not channels or channel in channels:` instead. That way, if you pass in an empty list of channels, the intent is the same as passing in None.

@@ +785,5 @@
> +        except Http404:
> +            pass
> +    if hasattr(settings, 'LOG_MOUNTS') and master in settings.LOG_MOUNTS:
> +        return generateLogFromFile(master, filename, channels)
> +    raise Http404

Please raise regular python error instead. It's not a client error. 

In fact, let's ride this which got landed in develop:
https://github.com/mozilla/elmo/commit/34492dc40e1a48086bf8a7b405c1a904ab04847c

Comment 8

[Peter Bengtsson [:peterbe]]

Comment on attachment 732377 [details] [diff] [review]
show logs from both ES and file

Review of attachment 732377 [details] [diff] [review]:
-----------------------------------------------------------------

pyelasticsearch needs to go into vendor-local/lib/python (and requirements/prod.txt accordingly)

::: apps/tinder/templates/tinder/showbuild.html
@@ +20,4 @@
>  <dt>{{ step.text|join:" " }}</dt>
>  <dd>
>    {% for log in step.logs.all %}
> + <a href="{% url 'tinder.views.showlog' build.builder.master.name,build.builder.name,build.buildnumber,step.name,log.name%}">{{log.name}}</a>

Why does this need to be one massive unspaced line?

::: apps/tinder/views.py
@@ +771,4 @@
>                      offset = 0
>                  else:
>                      offset += cnt
> +                if channels is None or channel in channels:

Can you make that `if not channels or channel in channels:` instead. That way, if you pass in an empty list of channels, the intent is the same as passing in None.

@@ +785,5 @@
> +        except Http404:
> +            pass
> +    if hasattr(settings, 'LOG_MOUNTS') and master in settings.LOG_MOUNTS:
> +        return generateLogFromFile(master, filename, channels)
> +    raise Http404

Please raise regular python error instead. It's not a client error. 

In fact, let's ride this which got landed in develop:
https://github.com/mozilla/elmo/commit/34492dc40e1a48086bf8a7b405c1a904ab04847c

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 732377 [details] [diff] [review]
show logs from both ES and file

Review of attachment 732377 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/tinder/views.py
@@ +722,5 @@
> +    except:
> +        # these should generate valid 500 errors, reraise
> +        raise
> +    if 'error' in hits or not hits.get('hits', {}).get('hits'):
> +        raise Http404

Note to self, review what to do here, iff.

Comment 10

[Axel Hecht [:Pike]]

So, this bug took a left turn. We're moving away from buildbot, so we're changing what we want to store in ES.

Instead of a bunch of log chunks to create the json and then the object with the comparison details, we'll save the actual details object in ES. That's the new plan.

mapping is looking like this now:

properties = {
    # the actual compare-locales data, just store as object
    "details": {
        "type": "object",
        "enabled": False
    },
    "run": {"type": "long"}
}

run is the db pk for the summary entry in elmo, and also the ID of the ES document.

Double needinfo for Erik:

Does that sound sound to you? I had chatted this through with jezdez, too.

Also, the final step to actually create a patch is to update the copies of the libraries in vendor-local. It's been a bit since pyelasticsearch released, is there a new coming up? Or would you cut one, given that the release notes changes are in?

The lengthy branch is building up on https://github.com/Pike/elmo/compare/feature;bug-857107-store-comparisons-in-es

Comment 11

[Erik Rose [:erik][:erikrose]]

1. Remember that ints in ES go up to 2B; you probably don't need a long for "run". And you might as well just use the "_id" field; you don't really gain anything by making a specially named field called "run": http://www.elasticsearch.org/guide/reference/mapping/id-field/. You may not need to mention an _id field at all in your mapping, depending on what you plan to do. I recommend reading that id-field page in detail.

2. "enabled": False looks funny to me. Based on http://www.elasticsearch.org/guide/reference/mapping/object-type/, I would expect the above to avoid indexing, storing, or doing anything at all with the "details" field—omitting it from a document with many more fields that you'll be passing in. Do you plan to index much larger documents with fields that aren't listed in the mapping?

3. I'm going to cut a pyelasticsearch release in the next day or 3. It's time to get that bulk-index fix out, if nothing else.

Comment 12

[Axel Hecht [:Pike]]

(In reply to Erik Rose [:erik][:erikrose] from comment #11)
> 1. Remember that ints in ES go up to 2B; you probably don't need a long for
> "run". And you might as well just use the "_id" field; you don't really gain
> anything by making a specially named field called "run":
> http://www.elasticsearch.org/guide/reference/mapping/id-field/. You may not
> need to mention an _id field at all in your mapping, depending on what you
> plan to do. I recommend reading that id-field page in detail.

'k

> 2. "enabled": False looks funny to me. Based on
> http://www.elasticsearch.org/guide/reference/mapping/object-type/, I would
> expect the above to avoid indexing, storing, or doing anything at all with
> the "details" field—omitting it from a document with many more fields that
> you'll be passing in. Do you plan to index much larger documents with fields
> that aren't listed in the mapping?

Well, most of the data is in the hierarchical object.

{"_index":"elmo-comparisons","_type":"comparison","_id":"5","_version":1,"exists":true, "_source" : {"run": 5, "details": {"children": [["de/browser/file.properties", {"value": {"obsoleteEntity": ["stuff"]}}]]}}}

is actually what I get out for http://localhost:9200/elmo-comparisons/comparison/5 in my local setup.

> 3. I'm going to cut a pyelasticsearch release in the next day or 3. It's
> time to get that bulk-index fix out, if nothing else.

Thanks

Comment 13

[Erik Rose [:erik][:erikrose]]

Huh, I'm surprised that you get anything out after saying enabled: False. Might want to make sure that doesn't change in the latest version of ES.

Also, is there a reason not to flatten the details object out into the top-level document namespace?

Comment 14

[Erik Rose [:erik][:erikrose]]

Shiny new pyelasticsearch for you: https://pypi.python.org/pypi/pyelasticsearch/

Comment 15

[Axel Hecht [:Pike]]

(In reply to Erik Rose [:erik][:erikrose] from comment #13)
> Huh, I'm surprised that you get anything out after saying enabled: False.
> Might want to make sure that doesn't change in the latest version of ES.
> 
> Also, is there a reason not to flatten the details object out into the
> top-level document namespace?

I pondered that, but it'd require that I change the code on both the writing and the reading side. Also the values are somewhat fancy themselves, so it might not win much.

I filed https://github.com/elasticsearch/elasticsearch.github.com/issues/486 to get the docs clarified.

(In reply to Erik Rose [:erik][:erikrose] from comment #14)
> Shiny new pyelasticsearch for you:
> https://pypi.python.org/pypi/pyelasticsearch/

Thanks.

Comment 16

[Axel Hecht [:Pike]]

Needinfo on Jannis and Erik, there's a new python binding by the ES folks themselves, and I'll need their guidance on whether to drop pyelasticsearch for that or not. Let's get 'em a flag to figure that out.

Comment 17

[Jannis Leidel [:jezdez]]

Just talked with :Pike about this and explained the current situation with pyelasticsearch/elasticsearch.py.

Comment 18

[Axel Hecht [:Pike]]

Making things a bit more granular, and I'll file a few follow-up bugs.

This bug is really only about letting us load compare-locales results from ES, as well as buildbot logs. Plus a command to copy the data from local logs over into ES.

Comment 19

[Axel Hecht [:Pike]]

Attachment: three changesets, let's see if linking a PR helps us

Let's see, does a review request on a PR make sense for us in this context?

I think it makes sense to land this work in three commits actually, thus not going for a single patch.

I could do the HEAD as attachment if that helps with the review.

Comment 20

[Axel Hecht [:Pike]]

I got a forwards-compatible bust, I'm checking "OK": true in the bulk stuff, and OK went away in 1.0.

https://groups.google.com/forum/?fromgroups=#!topic/elasticsearch/Npk7Y0a-sog is the mailing list thread, and the suggested path is to just not bother and use py-elasticsearch, which is supposed to have that covered. So I'll give that a try. It also seems that the helper api for incremental bulk could be really helpful in our case.

Comment 21

[Axel Hecht [:Pike]]

Attachment: actual implementation to store/load compare-locales in ES

Here's the tip patch for real review, this is based on one commit required for vendor-local that adds the python libs.

You can test the full branch on https://github.com/Pike/elmo/tree/feature/bug-857107-cl-py-elasticsearch

Comment 22

[Robert Helmer [:rhelmer]]

Comment on attachment 8447976 [details] [diff] [review]
actual implementation to store/load compare-locales in ES

The code looks fine to me - I am not really set up locally to be able to test this at the moment though :(

Comment 23

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #22)
> Comment on attachment 8447976 [details] [diff] [review]
> actual implementation to store/load compare-locales in ES
> 
> The code looks fine to me - I am not really set up locally to be able to
> test this at the moment though :(

OK I am set up to test this now :)

Axel, did you land this yet?

Comment 24

[[github robot]]

Commits pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/562fb8d2fd2c62c7996bd97190074cd119546a5e
bug 857107, add py-elasticsearch, comes with urllib3

py-elasticsearch: 1.0.0
urllib3: 1.8 (2014-03-04)

https://github.com/mozilla/elmo/commit/033b1f7b0b01c0f08be3801212f49707aca7e493
bug 857107, store and load compare-locales results via elasticsearch, r=rhelmer

Comment 25

[Axel Hecht [:Pike]]

Yes, this landed, marking FIXED.