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

Jump to bottom

x/build/cmd/gopherbot: auto-Subscribe owners to reported issues based on directory #27586

Open
kevinburke opened this issue Sep 9, 2018 · 18 comments
Open

x/build/cmd/gopherbot: auto-Subscribe owners to reported issues based on directory #27586

kevinburke opened this issue Sep 9, 2018 · 18 comments
Labels
Builders x/build issues (builders, bots, dashboards)
Milestone
Unreleased

Comments

@kevinburke
Copy link
Contributor

We now have an owners file which assigns various Gophers as owners/cc'd on issues for different directories.

It would be good to match incoming issues and auto-subscribe the relevant people, e.g. to do in an automatic fashion what @bcmills is doing manually here. #27524 (comment)
@gopherbot gopherbot added this to the Unreleased milestone Sep 9, 2018
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Sep 9, 2018
@mvdan
Copy link
Member

mvdan commented Sep 9, 2018

I assume this won't be done for the entire backlog of issues - it could mean hundreds of notifications for quite a few people :)

It would also be good if this was only done for issues that are still open after an amount of hours. Sometimes, issues are closed as invalid or duplicates after a few hours, and in those cases there's little point in pinging the owners.

Lastly: should this be skipped if any of the owners has already replied to the thread, or been pinged in it?

@agnivade
Copy link
Contributor

agnivade commented Sep 9, 2018

The challenge is to automatically figure out whom to ping from the issue title (which does not always have the package: prefix) and the issue body.

@josharian
Copy link
Contributor

I think gopherbot should wait for the issue to be manually retitled. This also helps with the issue raised by @mvdan.

@kevinburke
Copy link
Contributor Author

Lastly: should this be skipped if any of the owners has already replied to the thread, or been pinged in it?

I'm not sure there's an API for subscribing other people to issues so yeah my idea was just to have gopherbot mention the owners, which subscribes them, if they're not subscribed already.

I think if you are currently subscribed and then unsubscribe, Github is smart enough to know you shouldn't be resubscribed just because someone at-mentioned you.

I assume this won't be done for the entire backlog of issues - it could mean hundreds of notifications for quite a few people :)

Yeah agree that would be a bad idea.

@bcmills
Copy link
Contributor

bcmills commented Sep 10, 2018

I think gopherbot should wait for the issue to be manually retitled.

I don't think we need to wait if there is already a valid path in the title: folks who know to add the path prefix usually also have a pretty good idea which one.

@bcmills
Copy link
Contributor

bcmills commented Mar 4, 2019 •
edited

Sometimes, issues are closed as invalid or duplicates after a few hours, and in those cases there's little point in pinging the owners.

I've been thinking about this a bit.

There's not much point in waiting if a human has already looked at the issue and decided that it is valid. In practice, the NeedsInvestigation, NeedsDecision, NeedsFix labels indicate that a human has looked at the issue, and WaitingForInfo indicates whether the human has decided that the issue is complete enough for further discussion.

So I would say that GopherBot should auto-add owners if:

  • the issue is still open, and
  • the issue is not labled WaitingForInfo, and one of:
    • the issue is labeled NeedsInvestigation, NeedsDecision, or NeedsFix, or
    • the issue does not have a Needs label, and has been open and without the WaitingForInfo label for some interval of time (one business day?)

CC @golang/osp-team

@bcmills
Copy link
Contributor

bcmills commented Mar 4, 2019 •
edited

And we should probably only apply the “open and without label” condition to issues created after some epoch, or use a formula to limit the rate at which older issues are CC'd.

@bcmills
Copy link
Contributor

bcmills commented Apr 8, 2019

I'm on issue-triage interrupts this week and I'm tired of manually CC'ing owners, so I'm planning to implement this.

@bcmills bcmills self-assigned this Apr 8, 2019
@ianlancetaylor
Copy link
Contributor

Note that I find the CL auto-reviewers to be more spammy than useful. There's no point to CCing rsc on everything. But perhaps the idea can be more useful for issues as they are less likely to spread across multiple directories.

@bcmills
Copy link
Contributor

bcmills commented Apr 8, 2019

I suspect that we can prune down the CL auto-reviewers significantly: they're currently based on source code paths rather than CL-titled paths, whereas each issue generally only has a single path.

For issues in particular, I'm planning to auto-CC owners only after the issue has been triaged by a human, and to skip CC'ing issues that already have an assignee (since the assignee can contact anyone they want for assistance).

If we can stick to one path (or a small number of paths) per issue or CL, then the problem of a small number of people getting CC'd on everything boils down to one of distributing and focusing ownership: if one person is a primary owner for a significant fraction of issues or changes, that makes them a bottleneck for discussions and reviews, not just an unfortunate target for spammy updates.

@bcmills
Copy link
Contributor

bcmills commented Apr 9, 2019

My current plan for keeping the spam level down is to avoid adding the Secondary reviewers unless there are fewer than two Primary, and to use a one-year rolling window to add mentions to existing issues.

(Two seems like a good number, because we need one author and one reviewer to write a fix, and in general we need two knowledgeable people to be able to meaningfully discuss a potential solution.)

@bcmills
Copy link
Contributor

bcmills commented Apr 9, 2019

I'm also planning to start with a one-week idle window for issues that lack Needs labels. That way, we won't start pinging every issue filed over a holiday weekend, and don't need to hard-code Google's vacation calendar in gopherbot.

@gopherbot
Copy link

Change https://golang.org/cl/171036 mentions this issue: devapp/owners: add new owners on the Go team at Google

@gopherbot
Copy link

Change https://golang.org/cl/170863 mentions this issue: cmd/gopherbot: CC triaged issues to owners

gopherbot pushed a commit to golang/build that referenced this issue Apr 9, 2019
devapp/owners: add new owners on the Go team at Google
d3780c0 
Move rsc and iant to secondary for cmd/go (to reduce CC and
review-spam for them).

Updates golang/go#27586

Change-Id: I0601d4b50202708726a666d774413d90452c02cd
Reviewed-on: https://go-review.googlesource.com/c/build/+/171036
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/171242 mentions this issue: devapp/owners: use path prefixes instead of string prefixes for matching

@gopherbot
Copy link

Change https://golang.org/cl/171358 mentions this issue: devapp/owners: remove catchall entry for the 'go' repo

gopherbot pushed a commit to golang/build that referenced this issue Apr 10, 2019
devapp/owners: use path prefixes instead of string prefixes for matching
e95584b 
Remove trailing slashes uniformly from table entries.

Previously, we used string prefixes, which could over-match if two
packages' final elements share a prefix, and used trailing slashes to
compensate, but it's remarkably easy to omit a trailing slash and end
up with a pattern that usually-but-not-always works.

Updates golang/go#27586

Change-Id: Id029a706653e2ba42e83d68f789b65fca34be623
Reviewed-on: https://go-review.googlesource.com/c/build/+/171242
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Apr 10, 2019
devapp/owners: remove catchall entry for the 'go' repo
a541ae4 
The owners table is consumed by gopherbot to assign reviewers and
escalate triaged issues.

Gopherbot has its own fallback behavior for unowned paths, which
should generally be to escalate to whichever member of the Go team is
on triage duty (or to escalate to the Go Open Source Project subteam
in general to update the owners table).

Due to the explicit entry for the root of the 'go' repository, when
gopherbot encounters an issue or CL for any standard-library package
without an owner, it will instead escalate to rsc, ianlancetaylor, and
bradfitz. That doesn't seem like a great use of their time, and also
masks the missing entry in the owners table.

This CL removes that top-level entry, so that unknown paths within the
Go repo will appear to gopherbot as such.

Updates golang/go#27586

Change-Id: I469028f7ec5f4b357ae33bca125da2323fc3e0d2
Reviewed-on: https://go-review.googlesource.com/c/build/+/171358
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@bcmills bcmills mentioned this issue Apr 15, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/172278 mentions this issue: devapp/owners: add owners for build, debug, gccgo, tools, and go/src/cmd/asm

gopherbot pushed a commit to golang/build that referenced this issue Apr 16, 2019
devapp/owners: add owners for build, debug, gccgo, tools, and go/src/…
d37bb9a 
…cmd/asm

These components have had recent activity in the issue tracker, so
it's helpful to add explicit owners, even if their primary role as
such is to dispatch issues to more-specific owners who are not yet
listed.

Updates golang/go#27586

Change-Id: Ib77b6d1d02b49b9709f1f58797acf2b64ce95e48
Reviewed-on: https://go-review.googlesource.com/c/build/+/172278
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/217539 mentions this issue: devapp/owners, internal/gophers: add owners for {pkg,learn,}.go.dev

gopherbot pushed a commit to golang/build that referenced this issue Feb 5, 2020
@dmitshur
devapp/owners, internal/gophers: add owners for {pkg,learn,}.go.dev
a352cbf 
This information helps when looking up who to /cc when triaging issues.

Updates golang/go#33654
Updates golang/go#27586

Change-Id: I110a071c4644da38d33335460451731465fffcc1
Reviewed-on: https://go-review.googlesource.com/c/build/+/217539
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards)
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@josharian @kevinburke @agnivade @ianlancetaylor @mvdan @bcmills @gopherbot