Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/build/dashboard: repair perf builders #8930

Closed
dvyukov opened this issue Oct 14, 2014 · 14 comments
Closed

x/build/dashboard: repair perf builders #8930

dvyukov opened this issue Oct 14, 2014 · 14 comments
Assignees
Milestone

Comments

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Oct 14, 2014

The perf builders don't benchmark older commits:
http://build.golang.org/perf?page=100
and perf-todo requests from builders fail:
2014/10/14 12:54:05 datastore: query has no more results
2014/10/14 12:55:04 datastore: query has no more results
2014/10/14 12:55:34 datastore: query has no more results
2014/10/14 12:56:04 datastore: query has no more results
2014/10/14 12:56:35 datastore: query has no more results

Looking at the symptoms it seems that Commit.Num's were manually updated, so that now
(1) PerfTodo entities contain invalid nums and (2) association between Commits and
CommitRun's is broken and (3) Commit nums jumped to 100'000, which created a huge hole
in CommitRun's which negatively affects the graphs UI (one could argue that benchmark
commits contain holes due to branches anyway, but small holes are OK and 100'000 is like
25 years of team's activity).

All these 3 points need to be repaired.
@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented Oct 14, 2014

Comment 1:

Owner changed to @bradfitz.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2014

Comment 2:

Can you say why you think this is Brad's job? Don't you own the perf dashboard?
@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented Oct 14, 2014

Comment 3:

It's at least job for somebody who has access to the database.
As far as I understand Brad started doing manual updates to the database.
@adg
Copy link
Contributor

@adg adg commented Oct 14, 2014

Comment 4:

We need to devise a strategy for repairing the database. You suggested nuking the
existing CommitRun entities and re-building them. Is that still your view?

Owner changed to @adg.

@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented Oct 14, 2014

Comment 5:

I don't know how exactly the database is broken.
The nuke would repair (1) and (2). I am not sure whether (2) can be repaired w/o the
nuke.
But (1) is most easily repairable -- we can just drop all PerfTodo's.
As for (3), the following request is very slow now:
http://build.golang.org/perfgraph?builder=linux-amd64-perf&benchmark=garbage&benchmark=http&benchmark=json&procs=1&metric=time&startcommit=100043&commitnum=90000&zoomout=Zoom+out
It shows data from the current commit num 100043 back to 10043.
I am not sure whether doing more manual updates to repair it is a good idea. Maybe we
can just optimize it to the point where it has acceptable performance. It tries to
select 90x3 PerfResultRun's, but most of them do not exist (gap).
@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2014

Comment 6:

I did the manual edits, because everyone else was in Paris. It seems like you should be
able to live with the current data (I do not believe it is broken in any meaningful way;
there are just gaps) and fix the queries.
It seems like instead of using a Num lower bound to limit the results returned, you
should use an actual limit clause, ordering the result from newest to oldest.
@adg
Copy link
Contributor

@adg adg commented Oct 14, 2014

Comment 7:

@rsc: Dmitry is referring to the earlier edits that Brad made while both Dmitry and I
were unavailable. Brad doesn't remember exactly what he changed, so that's the big
question mark for us now.
I think re-building the CommitRuns should fix it though.
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 14, 2014

Comment 8:

Sorry, but I will not be spending any effort resurrecting the perf dashboard. I don't
like its design and that it's too intertwined with the build dashboard's code and data.
They should be separate apps.
If you'd like to redeploy a fresh instance of the perf dashboard to a different
golang.org subdomain, I will arrange for a commit watcher to ping your instance of new
commits.
But I see Andrew took this in the meantime, so he can decide how much patience he has.
@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2014

Comment 9:

Even better, let's deploy it somewhere else, so that it doesn't need to be in the same
datastore as the rest of golang.org. The subdomains are nice but they mean all the data
is mixed together in a not-good way. It's very fragile.
@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented Oct 15, 2014

Comment 10:

Brad, Russ,
What exactly is fragile? Why do you think intertwining is bad in this case? And why do
you say this only now?
The perf dashboard worked well for half a year on goperfd.appspot.com and then for few
months on build.golang.org. I don't see any issues, like fragility, with it.
The perf dashboard is intertwined with build dashboard because they participate in the
same workflows -- notifications about changes after new changes, checking status of
repo, trying new patches on builders. Separating them will just create lots of
additional work for everybody and will make user experience worse (e.g. you won't be
able to filter duplicate notifications about breakages coming from build and perf).
I don't see how moving it to a separate app will solve fragility or anything else.
If we want to make manual periodical updates of the database part of our workflow, we
just need to fix the scripts to make it correctly.
@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented Oct 18, 2014

Comment 11:

Brad removed all PerfTodo entities, but it did not help. Builders still say "datastore:
query has no more results". So it seems to be corrupted commit nums and commit runs.
I hope I fixed (3) by https://golang.org/cl/159910045/ (dashboard: faster perf
data fetching).
To fix (1) and (2) we need to fix database consistency somehow.
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 22, 2014

Comment 12:

CL https://golang.org/cl/158320043 mentions this issue.
adg pushed a commit to golang/build that referenced this issue Jan 21, 2015
This is required to repair perf data in the datastore.
Update golang/go#8930.

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/158320043
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc removed accepted labels Apr 14, 2015
@rsc rsc changed the title dashboard: repair perf builders x/build/dashboard: repair perf builders Apr 14, 2015
@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015
@rsc rsc removed the builder label Apr 14, 2015
@rsc rsc added the Builders label Jun 11, 2015
@adg adg removed their assignment May 27, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 6, 2017

I created #19871 to track the build coordinator changes.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 21, 2018

The performance dashboard was rewritten and deployed, but then had problems and was ownerless and so was disabled and abandoned for the second time. Re-enabling it is now #20552.

Closing this one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.