The search console page is broken, is not used or maintained, and is
only referenced by a series of blog posts years ago. We have product
support to remove it.
Currently, we do not respect the search.contextLines setting in the backend. Specifically, the search backend currently always yields results with zero lines of context.
One effect of this is in order to display matches with context lines, any client needs to make a followup request to fetch the full contents of the file. This makes the UI feel laggy because results filter in slowly as you scroll. This is exacerbated by the fact that we load the highlighted code, and highlighting can be unpredictable, sometimes taking a couple seconds to return.
We already stream the matched chunk back to the client, so this just updates the backend so that the streamed results include the number of context lines a user requested. Zoekt already supports this, so it was just a matter of taking advantage of that setting and updating searcher to do the same.
This will be my last PR related to the backend tracing work I've been
doing. This is a set of small cleanups to the `trace` package that I've
collecting as I have worked on tracing and used tracing. Following this
PR, the `trace` package is just a very lightweight wrapper around the
standard OpenTelemetry APIs. I think it's best to keep the package
around rather than using opentelemetry directly because it easy to add
convenience methods (which I would be sad to lose).
Each commit is self-contained and has a descriptive message.
If anyone wants to pick up where I'm leaving off, here are a few things
left undone:
- Convert Zoekt to use OpenTelemetry rather than OpenTracing
- Add OpenTelemetry support to other services like syntect-server
- Merge the `internal/trace` and `internal/tracer` packages
- Consider adding a type that conforms to the OpenTelemetry `Span`
interface but also writes to `x/net/trace` that can be enabled when
tracing is not available.
- Remove unrelated code from the `trace` and `tracer` package (see
[here](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@a6759b95dbd8e5e3a604f7fd452b0b85f37091d9/-/blob/internal/tracer/tracer.go?L75-83)
and
[here](https://sourcegraph.com/github.com/sourcegraph/sourcegraph@769fbbf5008e8decc63967dbff53f26333620265/-/blob/internal/trace/buckets.go?L3-7))
- Noodle on a `Traceable` interface (one that impls `Attr()` or
`Attrs()`) so types can be easily added with `SetAttributes()`
- Experiment with sampling
- Experiment with replacing `policy.ShouldTrace` with native
opentelemetry tools
## Test plan
Tested manually that tracing still looks good locally. Will test on
other instances when it rolls out.
<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->
This removes the "title" argument to `trace.New()`. This was previously
only displayed as an attribute on the span, which is confusing since
people would often use a span name for this argument since it is called
"title." OpenTelemetry does not have the concept of a family and title
(these are from `x/net/trace`, which is no longer used), so this is
another step towards reconciling our tracing package with OpenTelemetry.
Basically, I went through all uses of `trace.New()` and either named the
span `family.title`, removed the title in the case it was an empty
string, or converted the title to a set of more structured attributes.
Now, `trace.New()` only takes a context, a span name, and an optional
set of attributes.
We're getting a [concurrent transaction
error](https://sourcegraph.sentry.io/issues/4263654659/?project=6583153&referrer=slack)
from settings because somewhere we're passing a transaction as the
`database.DB` then using it concurrently in this method.
This updates the DB calls to run sequentially. This comes with a perf
penalty, but I'd prefer a small perf impact to
erroring/crashing/silently doing the wrong thing. As a followup, I will
add a DB method that allows you to request settings for multiple
subjects at once. That might not come until after 5.1 though, so I'd at
least like to get this merged and backported.
Originally I started working on this because of
[comment on another
PR](https://github.com/sourcegraph/sourcegraph/pull/53373#discussion_r1228058455).
I quickly wrote an implementation of Ptr and NonZeroPtr. Then I started
refactoring existing code only to find out that @efritz already beat me
to it. But Erics implementation was hidden in
internal/codeintel/resolvers/utils.go
Moved all of the Ptr functions to `pointers` package instead and
refactored all existing code that I could find to use it instead of
redefining the same functions all the time.
Usage is mostly in tests, so hopefully the impact is not as huge as the
diff size might suggest.
## Test plan
A whole lot of unit tests.
We introduce a service interface for calculating user (and other subjects) settings.
The intention is to make this another service we can inject.
Test Plan: CI
The previous approach to enable race detection was too radical and
accidently led to build our binaries with the race flage enabled, which
caused issues when building images down the line.
This happened because putting a `test --something` in bazelrc also sets
it on `build` which is absolutely not what we wanted. Usually folks get
this one working by having a `--stamp` config setting that fixes this
when releasing binaries, which we don't at this stage, as we're still
learning Bazel.
Luckily, this was caught swiftly. The current approach insteads takes a
more granular approach, which makes the `go_test` rule uses our own
variant, which injects the `race = "on"` attribute, but only on
`go_test`.
## Test plan
<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->
CI, being a main-dry-run, this will cover the container building jobs,
which were the ones failing.
---------
Co-authored-by: Alex Ostrikov <alex.ostrikov@sourcegraph.com>
This creates a new `internal/settings` package that handles the settings
cascade logic. This allows us to get a user's settings without a
dependency on the `graphqlbackend` settings resolvers.
