Enable streaming blame by default (#58582)

This has been implemented and run for a while on Sourcegraph.com. I think we should open it for all users, or remove the code for it.
Since it fixes performance issues, I would want to err on keeping and on for all.
This commit is contained in:
Erik Seliger 2023-11-27 10:08:36 +01:00 committed by GitHub
parent 7cdadb80ab
commit 5a70adce92
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 7 additions and 120 deletions

View File

@ -12,7 +12,6 @@ export const FEATURE_FLAGS = [
'admin-analytics-cache-disabled',
'search-input-show-history',
'search-results-keyboard-navigation',
'enable-streaming-git-blame',
'plg-enable-add-codehost-widget',
'accessible-file-tree',
'accessible-symbol-tree',

View File

@ -12,14 +12,7 @@ import { makeRepoURI } from '@sourcegraph/shared/src/util/url'
import { useObservable } from '@sourcegraph/wildcard'
import { requestGraphQL } from '../../backend/graphql'
import { useFeatureFlag } from '../../featureFlags/useFeatureFlag'
import type {
ExternalServiceKind,
FirstCommitDateResult,
FirstCommitDateVariables,
GitBlameResult,
GitBlameVariables,
} from '../../graphql-operations'
import type { ExternalServiceKind, FirstCommitDateResult, FirstCommitDateVariables } from '../../graphql-operations'
import { useBlameVisibility } from './useBlameVisibility'
@ -69,84 +62,6 @@ export interface BlameHunkData {
firstCommitDate: Date | undefined
}
const fetchBlameViaGraphQL = memoizeObservable(
({
repoName,
revision,
filePath,
sourcegraphURL,
}: {
repoName: string
revision: string
filePath: string
sourcegraphURL: string
}): Observable<BlameHunkData> =>
requestGraphQL<GitBlameResult, GitBlameVariables>(
gql`
query GitBlame($repo: String!, $rev: String!, $path: String!) {
repository(name: $repo) {
externalURLs {
url
serviceKind
}
firstEverCommit {
author {
date
}
}
commit(rev: $rev) {
blob(path: $path) {
blame(startLine: 0, endLine: 0) {
startLine
endLine
author {
person {
email
displayName
avatarURL
user {
username
displayName
avatarURL
}
}
date
}
message
rev
commit {
url
parents {
oid
}
}
}
}
}
}
}
`,
{ repo: repoName, rev: revision, path: filePath }
).pipe(
map(dataOrThrowErrors),
map(({ repository }) => {
const hunks = repository?.commit?.blob?.blame
const firstCommitDate = repository?.firstEverCommit?.author?.date
const externalURLs = repository?.externalURLs
if (hunks) {
return {
current: hunks.map(blame => addDisplayInfoForHunk(blame, sourcegraphURL)),
externalURLs,
firstCommitDate: firstCommitDate ? new Date(firstCommitDate) : undefined,
}
}
return { current: undefined, externalURLs: undefined, firstCommitDate: undefined }
})
),
makeRepoURI
)
interface RawStreamHunk {
author: {
Name: string
@ -176,9 +91,6 @@ interface RawStreamHunk {
* To reduce the backend pressure and improve the experience, this fetch
* implementation uses a SSE stream to load the blame hunks in chunks.
*
* It is controlled via the `enable-streaming-git-blame` feature flag and is
* currently not enabled by default.
*
* Since we also need the first commit date for the blame recency calculations,
* this implementation uses Promise.all() to load both data sources in parallel.
*/
@ -347,8 +259,6 @@ export const useBlameHunks = (
},
sourcegraphURL: string
): BlameHunkData => {
const [enableStreamingGitBlame, status] = useFeatureFlag('enable-streaming-git-blame')
const [isBlameVisible] = useBlameVisibility(isPackage)
const shouldFetchBlame = isBlameVisible && status !== 'initial'
@ -356,11 +266,9 @@ export const useBlameHunks = (
useMemo(
() =>
shouldFetchBlame
? enableStreamingGitBlame
? fetchBlameViaStreaming({ revision, repoName, filePath, sourcegraphURL })
: fetchBlameViaGraphQL({ revision, repoName, filePath, sourcegraphURL })
? fetchBlameViaStreaming({ revision, repoName, filePath, sourcegraphURL })
: of({ current: undefined, externalURLs: undefined, firstCommitDate: undefined }),
[shouldFetchBlame, enableStreamingGitBlame, revision, repoName, filePath, sourcegraphURL]
[shouldFetchBlame, revision, repoName, filePath, sourcegraphURL]
)
)

View File

@ -44,7 +44,6 @@ go_library(
"//internal/encryption/keyring",
"//internal/env",
"//internal/errcode",
"//internal/featureflag",
"//internal/gitserver",
"//internal/gitserver/gitdomain",
"//internal/httpcli",
@ -107,7 +106,6 @@ go_test(
"//internal/database",
"//internal/database/dbmocks",
"//internal/errcode",
"//internal/featureflag",
"//internal/gitserver",
"//internal/gitserver/gitdomain",
"//internal/httpcli",

View File

@ -16,7 +16,6 @@ import (
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/database"
"github.com/sourcegraph/sourcegraph/internal/errcode"
"github.com/sourcegraph/sourcegraph/internal/featureflag"
"github.com/sourcegraph/sourcegraph/internal/gitserver"
"github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain"
streamhttp "github.com/sourcegraph/sourcegraph/internal/search/streaming/http"
@ -30,11 +29,6 @@ import (
// before that.
func handleStreamBlame(logger log.Logger, db database.DB, gitserverClient gitserver.Client) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
flags := featureflag.FromContext(r.Context())
if !flags.GetBoolOr("enable-streaming-git-blame", false) {
w.WriteHeader(404)
return
}
tr, ctx := trace.New(r.Context(), "blame.Stream")
defer tr.End()
r = r.WithContext(ctx)

View File

@ -9,18 +9,18 @@ import (
"github.com/gorilla/mux"
"github.com/sourcegraph/log/logtest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/sourcegraph/sourcegraph/cmd/frontend/backend"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/database"
"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
"github.com/sourcegraph/sourcegraph/internal/errcode"
"github.com/sourcegraph/sourcegraph/internal/featureflag"
"github.com/sourcegraph/sourcegraph/internal/gitserver"
"github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain"
"github.com/sourcegraph/sourcegraph/internal/types"
"github.com/sourcegraph/sourcegraph/lib/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func setupMockGSClient(t *testing.T, wantRev api.CommitID, returnErr error, hunks []*gitserver.Hunk) gitserver.Client {
@ -122,19 +122,7 @@ func TestStreamBlame(t *testing.T) {
backend.Mocks.Repos = backend.MockRepos{}
})
ffs := featureflag.NewMemoryStore(nil, nil, map[string]bool{"enable-streaming-git-blame": true})
ctx := featureflag.WithFlags(context.Background(), ffs)
t.Run("NOK feature flag disabled", func(t *testing.T) {
rec := httptest.NewRecorder()
req, err := http.NewRequest(http.MethodGet, "/no-vars", nil)
require.NoError(t, err)
req = req.WithContext(context.Background()) // no feature flag there
gsClient := setupMockGSClient(t, "abcd", nil, hunks)
handleStreamBlame(logger, db, gsClient).ServeHTTP(rec, req)
assert.Equal(t, http.StatusNotFound, rec.Code)
})
ctx := context.Background()
t.Run("NOK no mux vars", func(t *testing.T) {
rec := httptest.NewRecorder()