1116601 - Too difficult to determine what option_collection_hash values mean

Status: RESOLVED FIXED

(Tree Management :: Treeherder: API, defect, P4)

Description

[William Lachance (:wlach)]

So each performance signature currently contains something called an "option_collection_hash", from which you should be able to determine whether that signature corresponds to opt, debug, asan, or pgo builds. The procedure for finding out what the values mean are:

1. Hit the https://treeherder.mozilla.org/api/optioncollection/ endpoint to get the possible hash values. Current output:

[
    {
        "id": 3,
        "option": 3,
        "option_collection_hash": "03abd064e50ec12b8c7309950268531d78c63f60"
    },
    {
        "id": 1,
        "option": 1,
        "option_collection_hash": "102210fe594ee9b33d82058545b1ed14f4c8206e"
    },
    {
        "id": 4,
        "option": 2,
        "option_collection_hash": "32faaecac742100f7753f0c1d0aa0add01b4046b"
    },
    {
        "id": 2,
        "option": 4,
        "option_collection_hash": "f69e1b00908837bf0550250abb1645014317e8ec"
    }
]

2. One-by-one, look up the option id values via this endpoint: https://treeherder.mozilla.org/api/option/<option id>/

Example output for https://treeherder.mozilla.org/api/option/3/

{
    "active_status": "active",
    "description": "fill me",
    "id": 3,
    "name": "asan"
}

--

This is pretty overly complicated, especially since there are only 4 possible values. Let's simplify things a little bit. It should be fairly inexpensive to produce an endpoint that gives this kind of output:

{
    "03abd064e50ec12b8c7309950268531d78c63f60": [
        {
            "active_status": "active",
            "description": "fill me",
            "name": "asan"
        }
    ],
    "102210fe594ee9b33d82058545b1ed14f4c8206e": [
        {
            "active_status": "active",
            "description": "fill me",
            "name": "opt"
        }
    ]
}

That is to say, a dictionary where the keys are option_collection_hash, and the values are options associated with the hash. Yes, this involves multiple database queries but it should be pretty cheap since we only have 4 hashes with one option each associated with them. If the option collection hash ever gets bigger we can always memcache the return value here, but I suspect that won't be necessary anytime soon.

Comment 1

[Cameron Dawson [:camd]]

We may consider using this to address this issue:
http://drf-toolbox.readthedocs.org/en/latest/nested.html

Comment 2

[Kyle Lahnakoski [:ekyle]]

I noticed that option!=id :

    {
        "id": 4,
        "option": 2,
        "option_collection_hash": "32faaecac742100f7753f0c1d0aa0add01b4046b"
    },
    {
        "id": 2,
        "option": 4,
        "option_collection_hash": "f69e1b00908837bf0550250abb1645014317e8ec"
    }

When I call https://treeherder.mozilla.org/api/option/4/ I believe I am getting the former.  But what is the "option" property?  Is that in use?

Comment 3

[William Lachance (:wlach)]

(In reply to Kyle Lahnakoski [:ekyle] from comment #2)
> I noticed that option!=id :
> 
>     {
>         "id": 4,
>         "option": 2,
>         "option_collection_hash": "32faaecac742100f7753f0c1d0aa0add01b4046b"
>     },
>     {
>         "id": 2,
>         "option": 4,
>         "option_collection_hash": "f69e1b00908837bf0550250abb1645014317e8ec"
>     }
> 
> When I call https://treeherder.mozilla.org/api/option/4/ I believe I am
> getting the former.  But what is the "option" property?  Is that in use?

Yes, this is quite confusing, I had to look at the source again to figure out the answer to your question. The "option" property refers to a key in the option table. So in fact you are getting the former, not the latter in your example. The "id" in each element returned by `https://treeherder.mozilla.org/api/optioncollection/` is just the primary integer key in the option collection table. It doesn't refer to anything and isn't used anywhere.

Comment 4

[Kyle Lahnakoski [:ekyle]]

Oh dear, now I am more confused!

For clarity, what is the name of the option with the hash "32faaecac742100f7753f0c1d0aa0add01b4046b"?

Thanks!

Comment 5

[William Lachance (:wlach)]

(In reply to Kyle Lahnakoski [:ekyle] from comment #4)
> Oh dear, now I am more confused!
> 
> For clarity, what is the name of the option with the hash
> "32faaecac742100f7753f0c1d0aa0add01b4046b"?
> 
> Thanks!

That would be option 2 (as listed), aka "debug":

https://treeherder.mozilla.org/api/option/2/

Comment 6

[William Lachance (:wlach)]

I think we genuinely need to fix this, it seems to be confusing people quite a bit. I'll take it.

Comment 7

[Mauro Doglio [:mdoglio]]

The root cause of such a confusion is probably the (not ideal) design of that part of the schema. At the moment it's something like
+--------+ +-----------------------+
|option  | |option_collection      |
|--------+ +-----------------------+
| id     | | id                    |
| name   | | option_id             |
+--------+ | option_collection_hash|
           +-----------------------+
with a unique(option_id,option_collection_hash) constraint
There's a  1-N relationship between option and option_collection

Further normalizing option_collection, we obtain a N-N relationship, let's call it option_collection_option

+--------+ +-----------------------+ +-------------------------+
|option  | |option_collection      | |option_collection_option |
|--------+ +-----------------------+ +-------------------------+
| id     | | option_collection_hash| | option_collection_id    |
| name   | +-----------------------+ | option_id               |
+--------+                           +-------------------------+
So now we have a N-N relationship between a collection and its options. option_collection_hash is now primary key of option collection and there's a unique(option_collection_id, option_id) constraint on option_collection_option.

:wlach what do you think about this?

Comment 8

[William Lachance (:wlach)]

(In reply to Mauro Doglio [:mdoglio] from comment #7)
> Further normalizing option_collection, we obtain a N-N relationship, let's
> call it option_collection_option
> 
> +--------+ +-----------------------+ +-------------------------+
> |option  | |option_collection      | |option_collection_option |
> |--------+ +-----------------------+ +-------------------------+
> | id     | | option_collection_hash| | option_collection_id    |
> | name   | +-----------------------+ | option_id               |
> +--------+                           +-------------------------+
> So now we have a N-N relationship between a collection and its options.
> option_collection_hash is now primary key of option collection and there's a
> unique(option_collection_id, option_id) constraint on
> option_collection_option.
> 
> :wlach what do you think about this?

Yes, this makes sense to me. However I don't think we strictly need to do this in order to fix this bug?

Comment 9

[William Lachance (:wlach)]

Attachment: Implement new optioncollection API

This new API returns results like this:

{
    "03abd064e50ec12b8c7309950268531d78c63f60": {
        "options": [
            {
                "name": "asan"
            }
        ]
    },
    "102210fe594ee9b33d82058545b1ed14f4c8206e": {
        "options": [
            {
                "name": "opt"
            }
        ]
    },
    "32faaecac742100f7753f0c1d0aa0add01b4046b": {
        "options": [
            {
                "name": "debug"
            }
        ]
    }
}

(I could have just included the names since only those are really unique/interesting at this point, but decided to keep things open in the interests of future extensibility)

Comment 10

[Mauro Doglio [:mdoglio]]

For consistency with the other endpoints, it would be nice if this list method returned a list of objects:
[
   {
     "option_collection_hash": "03abd064e50ec12b8c7309950268531d78c63f60",
     "options": [
         {"name": "asan"}
     ]
   },
   {
     "option_collection_hash": "102210fe594ee9b33d82058545b1ed14f4c8206e",
     "options": [
         {"name": "opt"}
     ]
   },
   {
     "option_collection_hash": "32faaecac742100f7753f0c1d0aa0add01b4046b",
     "options": [
         {"name": "debug"}
     ]
   }
]

I also find this structure to be more explicit, which is good for a (still) undocumented api :-)

Comment 11

[Kyle Lahnakoski [:ekyle]]

Is it possible to have the service perform the option_collection_hash lookup and return a list of named options instead of returning an option_collection_hash?  This would be clearer to the service consumers, and save them from performing (?inevitable?) joins on the client side.

Comment 12

[William Lachance (:wlach)]

(In reply to Kyle Lahnakoski [:ekyle] from comment #11)
> Is it possible to have the service perform the option_collection_hash lookup
> and return a list of named options instead of returning an
> option_collection_hash?  This would be clearer to the service consumers, and
> save them from performing (?inevitable?) joins on the client side.

I'm not sure what you mean, both my original implementation and Mauro's suggested modifications to the option collection hash endpoint do exactly this as far as I can tell? There should be no reason to do a "join" on the client side after this goes in: all the relevant data will be returned by the server in an easy-to-digest form. Let me know if I'm missing something...

Comment 13

[Kyle Lahnakoski [:ekyle]]

Sorry, I did not mean to be specific, I meant in general:

For example, [1] exposes an option_collection_hash.  Instead I suggest replacing that key/value with the options list it really represents.  I do not know where else hash is exposed, but I suggest it be replaced there also.  

Even more generally, I suggest hashes and keys referencing the dimension tables (the arms in a star/snowflake schema) be automatically expanded by the web service to their full form.  This will make the response clearer, reduce the join logic on the client side, and reduce the number of calls required to gather this meta-data.

[1] https://treeherder.mozilla.org/api/project/mozilla-inbound/performance_artifact/?id__gte=4000000&id__lte=4000000&count=1

Comment 14

[William Lachance (:wlach)]

(In reply to Kyle Lahnakoski [:ekyle] from comment #13)
> Sorry, I did not mean to be specific, I meant in general:
> 
> For example, [1] exposes an option_collection_hash.  Instead I suggest
> replacing that key/value with the options list it really represents.  I do
> not know where else hash is exposed, but I suggest it be replaced there
> also.  
> 
> Even more generally, I suggest hashes and keys referencing the dimension
> tables (the arms in a star/snowflake schema) be automatically expanded by
> the web service to their full form.  This will make the response clearer,
> reduce the join logic on the client side, and reduce the number of calls
> required to gather this meta-data.
> 
> [1]
> https://treeherder.mozilla.org/api/project/mozilla-inbound/
> performance_artifact/?id__gte=4000000&id__lte=4000000&count=1

Ah, I see. Yes, I generally agree with you there. Feel free to file a follow-up bug re: the performance artifacts if you like.

Comment 15

[William Lachance (:wlach)]

(In reply to Mauro Doglio [:mdoglio] from comment #10)
> For consistency with the other endpoints, it would be nice if this list
> method returned a list of objects:
> ...
> 
> I also find this structure to be more explicit, which is good for a (still)
> undocumented api :-)

I updated the API as such. I also rejiggered things so that this api is an addition to what we already have, rather than a replacement. This will ensure that existing consumers of the old API (i.e. Kyle) won't have their code suddenly break on them. Once they've had a chance to move over, we can remove the old /optioncollection and /option endpoints.

Comment 16

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder-service

https://github.com/mozilla/treeherder-service/commit/58632bc85b91475e2388767b613f77852a069ba3
Bug 1116601 - More intuitive optioncollection API

Comment 17

[Ed Morley [:emorley]]

(In reply to William Lachance (:wlach) from comment #15)
> I updated the API as such. I also rejiggered things so that this api is an
> addition to what we already have, rather than a replacement. This will
> ensure that existing consumers of the old API (i.e. Kyle) won't have their
> code suddenly break on them. Once they've had a chance to move over, we can
> remove the old /optioncollection and /option endpoints.

Kyle could you let us know when we're good to remove the old endpoint? :-)

Comment 18

[Kyle Lahnakoski [:ekyle]]

Please remove it.  

Technically my code uses this endpoint, but it is not running right now, and will be easy to replace when I get back to dzAlerts code.  When I do move dzAlerts it will be easier for me to identify a disabled endpoint than determining which (working) endpoint to use.

Thanks

Comment 19

[Ed Morley [:emorley]]

Great, thank you :-)

Will, are you ok doing that in this bug, or would you like me to file another?

Comment 20

[William Lachance (:wlach)]

Yeah, sorry, I meant to follow up with Kyle myself. Thanks for getting to this before me. :) I'll remove the endpoints as part of this bug.

Comment 21

[Ed Morley [:emorley]]

Np; I was just going through bugs that had bot commit comments but were still open, to see if there were some easy wins for reducing bug count :-)

Comment 22

[William Lachance (:wlach)]

Attachment: Remove obsolete option, optioncollection APIs

Comment 23

[Ed Morley [:emorley]]

Comment on attachment 8572116 [details] [review]
Remove obsolete option, optioncollection APIs

ty :-)

Comment 24

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-service

https://github.com/mozilla/treeherder-service/commit/1d51e58aeb2ac42b5393ae368b4673dbe636a1bf
Bug 1116601 - Remove obsolete option, optioncollection APIs

https://github.com/mozilla/treeherder-service/commit/e2bb3ff23f226813d5166968f5092f3d243c7a2e
Merge pull request #413 from wlach/1116601

 	Bug 1116601 - Remove obsolete option, optioncollection APIs

Comment 25

[William Lachance (:wlach)]

Looks like the sheriffing panel used the old API. Oops. We can simplify the code with the new stuff though, so hooray for that.

Comment 26

[William Lachance (:wlach)]

Attachment: Fix UI to use new optioncollectionhash api

Update UI frontend to use optioncollectionhash api's instead of deprecated option, optioncollection apis

Tested locally with a copy hacked up to use the sherriff panel, seems to work ok. :)

Comment 27

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/b9a9d5498efd6e1311776ce89e184e515bd601a4
Bug 1116601 - Update treeherder ui to use new optioncollectionhash API

Comment 28

[William Lachance (:wlach)]

Patch was applied and this is really fixed now.

Comment 29

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/5366e7edb5d623a0ca925ac97085152f45d99d27
Bug 1116601 - Update treeherder ui to use new optioncollectionhash API