1265709 - Reduce dependency of frontend on performance signature hashes

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

At the moment the performance data API (/api/<project>/performance/data/) returns data keyed by signature. E.g. if you asked for hash "d0704924de0ba1f74bf00577c3a21183363c70af" it would return a set of datums like this:

{
"d0704924de0ba1f74bf00577c3a21183363c70af": [datum1, datum2, ...]
}

This API doesn't currently support the case where the same hash belongs to multiple frameworks -- it'll just lump them all into the same blob in the return value (see bug 1265655). We can work around this by filtering on framework id as well, but it's still pretty confusing.

I think it would be better to create a new API which just indicates the exact signature id for each performance datum. That should remove a lot of the ambiguity here and make it harder for API consumers to shoot themselves in the foot.

This would also be a good opportunity to take better advantage of django rest framework's serializers and helper classes to make something less complicated and easier to maintain (we could keep the old one around for a while to support existing perfherder consumers).

Comment 1

[William Lachance (:wlach)]

It just occurred to me that another way of solving this would be to incorporate the framework id into the signature hash, then the signature hashes would always be unique. This would involve (once again, sigh) rewriting the performance signatures, but is probably worth it. Will think on this a bit more.

Comment 2

[William Lachance (:wlach)]

Taking this one because the compare view is now failing on stage due to the sheer number of signatures, which in turn causes us to hammer the API because we can only pass so many of them through to the endpoint.

Most endpoints allow you to pass in signature id these days, only the data one is missing it. I'll add the option and try to make most of the frontend use it.

Comment 3

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1265709 > mozilla:master

Comment 4

[William Lachance (:wlach)]

Comment on attachment 8927049 [details] [review]
[treeherder] wlach:1265709 > mozilla:master

This doesn't completely do away with all uses of the signature hash, but brings us closer.

Comment 5

[William Lachance (:wlach)]

P.S. I gave this a fair bit of testing on stage and it seems to work great.

Comment 6

[Robert Wood [:rwood]]

Comment on attachment 8927049 [details] [review]
[treeherder] wlach:1265709 > mozilla:master

Great! LGTM

Comment 7

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/e513dd9a2e31e8f78507b4687f984b756726e6b2
Bug 1265709 - Reference perf signatures by id instead of hash where possible (#2926)

* Make the performance data api accept signature id as input
* Rewrite most of the frontend to use that API in preference to passing
  in signature hashes
* Correspondingly increase the chunks of comparison data we fetch
  at once

Comment 8

[William Lachance (:wlach)]

Unfortunately I didn't quite manage to remove the use of signature hashes in the frontend completely so this bug is still valid:

https://github.com/mozilla/treeherder/blob/e513dd9a2e31e8f78507b4687f984b756726e6b2/ui/js/models/perf/alerts.js#L26

It might be worth considering solving this bug as part of a larger refactor, but it will probably have to be someone who is not me who does that.

Comment 9

[Ed Morley [:emorley]]

Is it worth morphing this into just what landed, and opening a new bug with a revised comment 0? (The current description was written 2 years ago, so presumably might need some tweaks even without the landing)

Comment 10

[William Lachance (:wlach)]

(In reply to Ed Morley [:emorley] from comment #9)
> Is it worth morphing this into just what landed, and opening a new bug with
> a revised comment 0? (The current description was written 2 years ago, so
> presumably might need some tweaks even without the landing)

Yeah, I think that might be a good idea. My thinking on this topic has changed a bunch since I wrote that-- I'll do something up later today.

Comment 11

[William Lachance (:wlach)]

Resolving this, and filed bug 1416861 for what I think are the next steps.