1419590 - Slow API endpoint /api/releases

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), enhancement, P1)

Description

[Nick Thomas [:nthomas] (UTC+12)]

When I load https://aus4-admin.mozilla.org/releases there's a delay while it looks up the list of releases via https://aus4-admin.mozilla.org/api/releases - about 12 seconds talking to prod, and 4 or so seconds talking to local docker. It may be the continual addition of releases, but it would be good to make sure we're not doing something expensive.

The data returned looks like
{
  "releases": [
    {
      "data_version": 1, 
      "name": "Asyncrendering-2.0", 
      "product": "SystemAddons", 
      "read_only": false, 
      "required_signoffs": {}, 
      "rule_ids": []
    }, 
...
so it could be the signoff and mapping lookup, or overall load on the DB.

Comment 1

[[e-mail] (:bhearsum)]

This is because of the signoff look-ups - I remember noticing the change when that landed, but I didn't think it was a big enough problem to block on.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

It's not the end of the world, just a bit of a wait. Perhaps we're checking for scheduled changes for each of the 1545 releases, and could use one query and some code instead.

Comment 3

[[e-mail] (:bhearsum)]

I see this mentioned almost daily as annoyance now, we should probably do something about it. The quick fix might be to disable signoff look-ups on /releases, which would probably be a worthwhile trade-off.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

Here's a request with some light instrumentation:

balrogadmin_1  | 2018-01-08 01:38:08,855 - WARNING - PID: 419 - Request: 139996151769488 - ReleasesAPIView.get#365: Getting release list
balrogadmin_1  | 2018-01-08 01:38:08,912 - WARNING - PID: 419 - Request: 139996151769488 - ReleasesAPIView.get#367: Got list, getting signoff requirements
balrogadmin_1  | 2018-01-08 01:38:16,153 - WARNING - PID: 419 - Request: 139996151769488 - ReleasesAPIView.get#372: Got signoffs, starting serializing
balrogadmin_1  | 2018-01-08 01:38:16,157 - WARNING - PID: 419 - Request: 139996151769488 - auslib.web.common.releases.serialize_releases#48: Done serialising, just need to jsonify now
balrogadmin_1  | [pid: 419|app: 0|req: 6/6] 172.18.0.5 () {54 vars in 1001 bytes} [Mon Jan  8 01:38:08 2018] GET /api/releases => generated 4348 bytes in 7404 msecs (HTTP/1.1 200) 8 headers in 291 bytes (1 switches on core 0)

so definitely a problem with looking for pending signoffs. If I disable those lookups then page load time (for the 256 releases in a local dev db) drops from 7s to ~200ms. Production has 1618 releases and page load is ~20s.

The request is handled by https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/releases.py#L367, and getPotentialRequiredSignoffs() is at https://github.com/mozilla/balrog/blob/master/auslib/db.py#L1880. It's not set up to use any caching. I wondered if it would be possible to use releases.isMappedTo() to short-circuit DB lookups, but it does uncached DB lookups too. 

So lets go back a few steps and flip this around - is there any reason we can't we look for incomplete scheduled changes for releases, and only then check for pending signoffs for that subset of releases ? With our current usage the subset will be 0 or very small, rather than scale linearly as we add more releases.

'name' is primary key in releases, and IIRC used for matching rows between releases and releases_scheduled_changes, so can the name be modified in a change ? Seems like that would be bad.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #4)
> Here's a request with some light instrumentation:
> 
> balrogadmin_1  | 2018-01-08 01:38:08,855 - WARNING - PID: 419 - Request:
> 139996151769488 - ReleasesAPIView.get#365: Getting release list
> balrogadmin_1  | 2018-01-08 01:38:08,912 - WARNING - PID: 419 - Request:
> 139996151769488 - ReleasesAPIView.get#367: Got list, getting signoff
> requirements
> balrogadmin_1  | 2018-01-08 01:38:16,153 - WARNING - PID: 419 - Request:
> 139996151769488 - ReleasesAPIView.get#372: Got signoffs, starting serializing
> balrogadmin_1  | 2018-01-08 01:38:16,157 - WARNING - PID: 419 - Request:
> 139996151769488 - auslib.web.common.releases.serialize_releases#48: Done
> serialising, just need to jsonify now
> balrogadmin_1  | [pid: 419|app: 0|req: 6/6] 172.18.0.5 () {54 vars in 1001
> bytes} [Mon Jan  8 01:38:08 2018] GET /api/releases => generated 4348 bytes
> in 7404 msecs (HTTP/1.1 200) 8 headers in 291 bytes (1 switches on core 0)
> 
> so definitely a problem with looking for pending signoffs. If I disable
> those lookups then page load time (for the 256 releases in a local dev db)
> drops from 7s to ~200ms. Production has 1618 releases and page load is ~20s.
> 
> The request is handled by
> https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/
> releases.py#L367, and getPotentialRequiredSignoffs() is at
> https://github.com/mozilla/balrog/blob/master/auslib/db.py#L1880. It's not
> set up to use any caching. I wondered if it would be possible to use
> releases.isMappedTo() to short-circuit DB lookups, but it does uncached DB
> lookups too. 
> 
> So lets go back a few steps and flip this around - is there any reason we
> can't we look for incomplete scheduled changes for releases, and only then
> check for pending signoffs for that subset of releases ? With our current
> usage the subset will be 0 or very small, rather than scale linearly as we
> add more releases.

This is a good idea.

> 'name' is primary key in releases, and IIRC used for matching rows between
> releases and releases_scheduled_changes, so can the name be modified in a
> change ? Seems like that would be bad.

You can't change PK columns in a Scheduled Change, so we don't need to worry about this case.

Comment 6

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/balrog/pull/462

Comment 7

[Collins Abitekaniza]

@bhearsum @nthomas I've just created a PR that uses one query instead of using for loops when getting potentialRequiredSignoffs for releases. This significantly makes the endpoint alot more faster. You could maybe take a look and provide some feedback https://github.com/mozilla/balrog/pull/462

Comment 8

[[github robot]]

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

https://github.com/mozilla/balrog/commit/457eb0d0e672109fe63b523a4e5c031f7316bb49
bug 1419590: Slow API endpoint /api/releases (#462). r=bhearsum

Comment 9

[[e-mail] (:bhearsum)]

In production and working well - thank you Collins!

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

So great to have this fast again, thanks!

Comment 11

[Collins Abitekaniza]

(In reply to Ben Hearsum (:bhearsum) from comment #9)
> In production and working well - thank you Collins!

(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #10)
> So great to have this fast again, thanks!

Ur welcome!