own: index recent contributors for ownership signals #51188

coury-clark · 2023-04-26T23:01:58Z

This PR hooks up the recent contributors indexer with the background job, effectively enabling indexing once the feature flag is switched on.

Test plan

You can start Sourcegraph and let the jobs auto-index, or you can manually control the queue through the own_background_jobs table.

Here is some example output auto-indexing the local dev repos:

[worker] INFO worker.own-repo-indexing-queue.own.background.index.scheduler.recent-contributors background/scheduler.go:84 Scheduling repo indexes for own job {"job-name": "recent-contributors"}
[worker] INFO worker.own-repo-indexing-queue.own.background.index.scheduler.recent-contributors background/scheduler.go:96 Own index job scheduled {"job-name": "recent-contributors", "row-count": 11}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"TraceId": "1e151b9be9a6cb308ab77719c6ad54c0", "SpanId": "e738c4f0d1af8d29", "name": "own_background_worker", "id": 123}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"name": "own_background_worker", "id": 124}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"TraceId": "4a857e335aa28ba2717c951cfff07ba5", "SpanId": "04e3ff4d85cb72bc", "name": "own_background_worker", "id": 125}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"name": "own_background_worker", "id": 126}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"TraceId": "e1b51f47de0dcbfdebd14bfc346f60a2", "SpanId": "0384937856ac05a4", "name": "own_background_worker", "id": 127}
[worker] INFO worker.own-repo-indexing-queue.Handle background/recent_contributors.go:67 commits inserted {"TraceId": "e1b51f47de0dcbfdebd14bfc346f60a2", "SpanId": "4c39331206218a30", "handle": {"count": 0, "repo_id": 6}}
[worker] INFO worker.own-repo-indexing-queue.Handle background/recent_contributors.go:67 commits inserted {"TraceId": "4a857e335aa28ba2717c951cfff07ba5", "SpanId": "f7bbf2aa07a3aef2", "handle": {"count": 0, "repo_id": 5}}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"name": "own_background_worker", "id": 128}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"name": "own_background_worker", "id": 129}
[worker] INFO worker.own-repo-indexing-queue.Handle background/recent_contributors.go:67 commits inserted {"handle": {"count": 0, "repo_id": 4}}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"name": "own_background_worker", "id": 130}
[worker] INFO worker.own-repo-indexing-queue.Handle background/recent_contributors.go:67 commits inserted {"handle": {"count": 0, "repo_id": 8}}
[worker] INFO worker.own-repo-indexing-queue.Handle background/recent_contributors.go:67 commits inserted {"handle": {"count": 1, "repo_id": 1}}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"name": "own_background_worker", "id": 131}
[worker] INFO worker.own-repo-indexing-queue workerutil/worker.go:326 Dequeued record for processing {"name": "own_background_worker", "id": 132}
[worker] INFO worker.own-repo-indexing-queue.Handle background/recent_contributors.go:67 commits inserted {"TraceId": "1e151b9be9a6cb308ab77719c6ad54c0", "SpanId": "b3615fd2c3989b98", "handle": {"count": 3, "repo_id": 11}}
[worker] INFO worker.own-repo-indexing-queue.Handle background/recent_contributors.go:67 commits inserted {"handle": {"count": 0, "repo_id": 9}}

# Conflicts: # internal/database/schema.json # internal/database/schema.md # migrations/BUILD.bazel # migrations/frontend/squashed.sql

# Conflicts: # internal/database/recent_contribution_signal.go

coury-clark · 2023-05-01T22:28:40Z

enterprise/internal/own/background/recent_contributors_test.go

+	"github.com/sourcegraph/sourcegraph/internal/types"
+)
+
+func Test_RecentContributorIndexFromGitserver(t *testing.T) {


This is largely the same test from the store, but feeding a mock gitserver through to the DB. It effectively tests the entire flow, except the queue.

coury-clark · 2023-05-01T22:29:32Z

internal/gitserver/commands.go

@@ -324,6 +324,63 @@ func (c *clientImplementor) CommitGraph(ctx context.Context, repo api.RepoName,
 	return gitdomain.ParseCommitGraph(strings.Split(string(out), "\n")), nil
 }

+func (c *clientImplementor) CommitLog(ctx context.Context, repo api.RepoName, after time.Time) ([]CommitLog, error) {
+	args := []string{"log", "--pretty=format:%H<!>%ae<!>%an<!>%ad", "--name-only", "--topo-order", "--no-merges"}


I added no merges, does that match your thinking @cbart?

Makes total sense to me

internal/gitserver/commands.go

sourcegraph-bot · 2023-05-01T23:26:14Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 931d6ad...afdd075.

Notify	File(s)
@indradhanush	internal/gitserver/client.go internal/gitserver/commands.go internal/gitserver/commands_test.go internal/gitserver/mocks_temp.go
@sashaostrikov	internal/gitserver/client.go internal/gitserver/commands.go internal/gitserver/commands_test.go internal/gitserver/mocks_temp.go

sashaostrikov

Nice! I left some comments, I guess the main thing is to add some test coverage for a new gitserver command

enterprise/internal/own/background/background.go

sashaostrikov · 2023-05-02T10:00:21Z

enterprise/internal/own/background/background.go

 }

-func (h *handler) Handle(ctx context.Context, logger log.Logger, record *Job) error {
+func (h *handler) Handle(ctx context.Context, lgr log.Logger, record *Job) error {


nit: we have a bit of inconsistent logger naming, in structs it is logger, in function parameters I can see lgr, should we unify it?

I guess lgr is to fix the prefix of logger clashing with the imported log package? At least this is what Goland usually complains to me about.

Yeah, when you have local variables that shadow the package name. It's annoying because sometimes we have the import alias log and other times logger, so I've been trying something new with lgr to always have a consistent import logger.

yep, makes sense

enterprise/internal/own/background/recent_contributors.go

internal/database/recent_contribution_signal.go

sashaostrikov · 2023-05-02T10:09:12Z

internal/gitserver/commands.go

@@ -324,6 +324,63 @@ func (c *clientImplementor) CommitGraph(ctx context.Context, repo api.RepoName,
 	return gitdomain.ParseCommitGraph(strings.Split(string(out), "\n")), nil
 }

+func (c *clientImplementor) CommitLog(ctx context.Context, repo api.RepoName, after time.Time) ([]CommitLog, error) {
+	args := []string{"log", "--pretty=format:%H<!>%ae<!>%an<!>%ad", "--name-only", "--topo-order", "--no-merges"}


Makes total sense to me

internal/gitserver/commands.go

indradhanush · 2023-05-02T14:07:53Z

internal/gitserver/commands.go

@@ -324,6 +324,63 @@ func (c *clientImplementor) CommitGraph(ctx context.Context, repo api.RepoName,
 	return gitdomain.ParseCommitGraph(strings.Split(string(out), "\n")), nil
 }

+func (c *clientImplementor) CommitLog(ctx context.Context, repo api.RepoName, after time.Time) ([]CommitLog, error) {
+	args := []string{"log", "--pretty=format:%H<!>%ae<!>%an<!>%ad", "--name-only", "--topo-order", "--no-merges"}


Drive by review

Have we considered benchmarking this function to see how this would perform for monorepos with 500k to 1M commits? Asking because we recently saw a bunch of customer issues where load was the primary bottleneck and it seems like this command can be pretty open ended and in the worst case list all the repo's commits?

I'm trying to anticipate problems that could occur in the future but if you all don't see an issue here - feel free to ignore.

Also the benchmarking (if it makes sense) is not a blocking review for this PR.

@indradhanush yep, definitely something we are going to test once this can go onto a customer-like instance. Most likely we will end up paging this anyway so we aren't always running the log from HEAD (this is how Code Insights works as well, we log from older commits to get reference hashes)

Co-authored-by: Alex Ostrikov <alex.ostrikov@sourcegraph.com>

sashaostrikov · 2023-05-03T13:56:52Z

internal/gitserver/commands_test.go

@@ -3206,3 +3208,54 @@ func TestBlameHunkReader(t *testing.T) {
 		}
 	})
 }
+
+func Test_CommitLog(t *testing.T) {


🚀🚀🚀

sashaostrikov

cbart and others added 19 commits April 20, 2023 14:13

Pull migration from #50727

30b76c2

Migrations and slightly massaged stores

7b85767

Experimenting with inserting paths

73e0ff9

Clean up commit_signals.go

e5bfdc9

rename RecentContributionSignalStore

6480636

Tidy and comment commit_signals.go, reverse index on aggregate

98c919c

Make up.sql idempotent

be338f1

Rename commit_signals to recent_contribution_signal

ee0dd16

Update BUILD files

321358b

Sync main

ec01a80

bazel configure again

543a16a

Merge branch 'main' into cclark/fixup-cezarys-branch

614333a

# Conflicts: # internal/database/schema.json # internal/database/schema.md # migrations/BUILD.bazel # migrations/frontend/squashed.sql

pr comments

81bbaab

wip

11d0960

wip

1ee3bdc

convert commit column to bytea

ee4bf6b

wip

f087f69

wip

3b2956f

Merge branch 'main' into cclark/own/wire-up-indexer

075090f

# Conflicts: # internal/database/recent_contribution_signal.go

cla-bot bot added the cla-signed label Apr 26, 2023

cbart and others added 8 commits April 30, 2023 23:04

Remove transactions from commit insertion

eee5961

remove comment

1590f83

refactor for testing

e52ea14

test for indexer

610e952

remove print

bef3b2a

wrap error

e43d448

add counter

0839bc1

Merge branch 'main' into cclark/own/wire-up-indexer

ac435f4

coury-clark self-assigned this May 1, 2023

coury-clark requested a review from cbart May 1, 2023 22:22

coury-clark requested a review from sashaostrikov May 1, 2023 22:22

cleanup

db4cdfc

coury-clark commented May 1, 2023

View reviewed changes

internal/gitserver/commands.go Show resolved Hide resolved

coury-clark changed the title ~~cclark/own/wire up indexer~~ own: index recent contributors for ownership signals May 1, 2023

coury-clark marked this pull request as ready for review May 1, 2023 23:24

bazel

768f362

sashaostrikov reviewed May 2, 2023

View reviewed changes

indradhanush reviewed May 2, 2023

View reviewed changes

coury-clark and others added 4 commits May 2, 2023 10:42

Apply suggestions from code review

f3b2a74

Co-authored-by: Alex Ostrikov <alex.ostrikov@sourcegraph.com>

Apply suggestions from code review

e33d681

Co-authored-by: Alex Ostrikov <alex.ostrikov@sourcegraph.com>

pr comments

d2a2fd7

tests and fixes

7b6e8ce

coury-clark requested a review from sashaostrikov May 2, 2023 20:55

bazel

afdd075

sashaostrikov reviewed May 3, 2023

View reviewed changes

sashaostrikov approved these changes May 3, 2023

View reviewed changes

coury-clark merged commit c08e946 into main May 3, 2023

coury-clark deleted the cclark/own/wire-up-indexer branch May 3, 2023 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

own: index recent contributors for ownership signals #51188

own: index recent contributors for ownership signals #51188

coury-clark commented Apr 26, 2023 •

edited

Loading

coury-clark May 1, 2023

coury-clark May 1, 2023

sashaostrikov May 2, 2023

sourcegraph-bot commented May 1, 2023 •

edited

Loading

sashaostrikov left a comment

sashaostrikov May 2, 2023

coury-clark May 2, 2023

sashaostrikov May 3, 2023

sashaostrikov May 2, 2023

indradhanush May 2, 2023

coury-clark May 2, 2023

sashaostrikov May 3, 2023

sashaostrikov left a comment

own: index recent contributors for ownership signals #51188

own: index recent contributors for ownership signals #51188

Conversation

coury-clark commented Apr 26, 2023 • edited Loading

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcegraph-bot commented May 1, 2023 • edited Loading

sashaostrikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashaostrikov left a comment

Choose a reason for hiding this comment

coury-clark commented Apr 26, 2023 •

edited

Loading

sourcegraph-bot commented May 1, 2023 •

edited

Loading