This PR aims to craft the /internal/tenant package for use by all Sourcegraph cluster-internal services to properly scope data visibility to the correct requesting tenant.
For now, we only expose methods we know we will DEFINITELY need.
This PR also adds the required middlewares so we can start to tinker with it in implementations.
## Test plan
CI passes. We don't enforce anything for now except not passing unparseable tenant IDs, which should be fine.
During an investigation, we saw that Rockskip was not using scip-ctags
for symbol parsing when applicable. This means that
1. Rockskip is getting less than optimal symbols for certain languages
(like Go)
2. Rockskip is getting no symbols for languages not in universal ctags
(Magik)
This PR attempts to solve this problem but updating Rockskip to re-use
the ctags parser pool logic from symbol service.
### Key Changes
- Update parser pool to be re-usable
- Push common logic for parser type detection into the parser pool
module
- Update rockskip service config to take a parser pool
- Update and add unit/integration tests
## Questions
- What performance impact will using this pooled parser have compared to
its previous behavior of spawning a new ctags process each time?
## Test plan
- [x] Add unit tests
- [x] Update integration tests
- [x] Manually test rockskip
- [x] Manually test symbolservice (in case of regression)
---------
Co-authored-by: Keegan Carruthers-Smith <keegan.csmith@gmail.com>
This PR restructures the packages to move all symbols-only code into the
symbols service. This helps to reason better about which service is
accessing what datastores.
Test plan:
Just moved code, compiler and CI are happy.
This PR tinkers a bit with building a test helper to run integration
tests that are still ~lightweight against a real gitserver.
The caller can either clone a real repo to disk / embed it in the git
repo, or can create a small repo on the fly, and then get a running
gitserver gRPC server that returns all the data required.
These tests should only exist outside of cmd/ and internal/, as there is
a big potential to do cross-cmd imports from here, which can cause bad
coupling. But for just these tests, that should be fine.
The most trivial rockskip indexing job that I put in here to POC this
runs in 6.3s, including all setup and teardown. That seems very
reasonable to me.
Test plan:
The POC test passes.
This PR is a result/followup of the improvements we've made in the [SAMS repo](https://github.com/sourcegraph/sourcegraph-accounts/pull/199) that allows call sites to pass down a context (primarily to indicate deadline, and of course, cancellation if desired) and collects the error returned from `background.Routine`s `Stop` method.
Note that I did not adopt returning error from `Stop` method because I realize in monorepo, the more common (and arguably the desired) pattern is to hang on the call of `Start` method until `Stop` is called, so it is meaningless to collect errors from `Start` methods as return values anyway, and doing that would also complicate the design and semantics more than necessary.
All usages of the the `background.Routine` and `background.CombinedRoutines` are updated, I DID NOT try to interpret the code logic and make anything better other than fixing compile and test errors.
The only file that contains the core change is the [`lib/background/background.go`](https://github.com/sourcegraph/sourcegraph/pull/62136/files#diff-65c3228388620e91f8c22d91c18faac3f985fc67d64b08612df18fa7c04fafcd).
This commit refactors the Rockskip metrics logic to use `observationCtx`. In
tests, we now pass `TestContext` which has a no-op metrics registerer.
This change lets us run more than one test that spins up a Rockskip service.
Without it, tests fail with "duplicate metrics registered" errors.
This PR adds an initializer function that will be called from the svcmain package through the call to tracer.Init.
The reasoning behind this is that it's very easy to accidentally use a package that uses conf internally from a service like Cody Gateway, Appliance, Migrator, or other MSP services and just because we don't have any config source for the trace ID should not let the process stall entirely.
To make it absolutely clear that a dependency is safe to use from a conf perspective, this change indicates that well by dropping the dependency on it entirely and making it another packages concern to pass this type down.
Test plan:
All tests for Sourcegraph are still passing.
---------
Co-authored-by: Robert Lin <robert@bobheadxi.dev>
We are getting "fatal error: concurrent map writes" in production when
enabling rockskip at large customers. The root cause is the repoToSize
map which is read for every search request (and written every "cache
miss").
This was the easiest change to make. We should probably also be single
flighting this request, but that can be changed at another time.
Additionally fixed another bug where in the cache miss case we would do
`size :=` and therefore not update size from 0, which then does a
sqliteSearch. This means on the very first request to a rockskip repo we
would always do sqliteSearch which is also a waste of resources.
Test Plan: Stefan manually testing :)
This PR removes the unused `ctagsBinary` field from symbols server and its
set-up commands. The binary that's actually used is loaded through a separate
codepath.
This PR consolidates two methods that were effectively doing the same thing: `ReadFile` and `NewFileReader` - where `ReadFile` used the latter under the hood, and wrapped an `io.ReadAll`.
Given entire files are returned from this function that can be of arbitrary size, exposing a simple high-memory-use footgun made me uneasy here, and having to use io.ReadAll explicitly as the consumer hopefully makes the person using this API feel the same way.
Sometimes, we can currently not avoid it (GQL layer), but where possible, using the reader is very likely the better option.
Since there would've been a slight change in how errors are reported, I also fixed that. Previously, `NewFileReader` would NOT return `os.ErrNotExist` errors, those would only be discovered when the returned reader is called.
Now, like `ReadFile`, and like `os.Open` we immediately return that error.
This comes at the cost of making a second gRPC request. This is not great, but we will migrate the FileReader API to a proper gRPC hopefully ASAP, which will let us optimize it better on the server-side.
Since Varun also asked for perf improvements here, this is probably one of the first things that would be easy to switch over to go-git, eliminating probably 50%+ of the overhead here.
Cody no longer needs it and it is obsolete now!
Since App added a non-insignificant amount of new concepts and alternative code paths, I decided to take some time and remove it from out codebase.
This PR removes ~21k lines of code. If we ever want parts of single binary (app), the redis kv alternatives, or the release pipeline for a native mac app back, we can look back at this PR and revert parts of it, but maintaining 21k lines of code and many code paths for which I had to delete a surprisingly small amount of tests justifies this move for me very well.
Technically, to some extent SG App and Cody App both still existed in the codebase, but we don't distribute either of them anymore, so IMO we shouldn't keep this weight in our code.
So.. here we go.
This should not affect any of the existing deployments, we only remove functionality that was special-cased for app.
* log: remove use of description paramter in Scoped
* temporarily point to sglog branch
* bazel configure + gazelle
* remove additional use of description param
* use latest versions of zoekt,log,mountinfo
* go.mod
Follow up and stacked on #54850.
**Highlight of this change**
Searcher now initiates a connection to the DB. There's a [separate
commit](a90cd139f8)
just for this change to make it easier to review.
Deleted a bunch of code in search/decorate.go and related tests because it
was not being used.
Part of #54741.
Co-authored-by: Erik Seliger <erikseliger@me.com>
We had a slight regression on app we would get the following failure
```
1685704884811209000 FATAL symbols.sqlite.setup shared/sqlite.go:68 github.com/sourcegraph/sourcegraph/cmd/symbols/shared.SetupSqlite failed to create parser pool {"Resource": {"service.name": "sourcegraph-backend-x86_64-apple-darwin", "service.version": "2023.0.0+dev", "service.instance.id": "Williams-MacBook-Pro"}, "Attributes": {"error": "failed to create new ctags parser: exec: \"universal-ctags\": executable file not found in $PATH"}}
```
The actual failure isn't that `universal-ctags` isn't available, it's
because `scip-ctags` isn't available and with an empty bin, `go-ctags`
uses `universal-ctags` expecting it to be on the path.
Short term:
1. On App, we only register the `universal-ctags` parser pool
2. Everything else uses `universal-ctags` and `scip-ctags`
Longer term:
1. Bundle `scip-ctags` with app, but that requires cross compiling it
for 3 platforms.
I also cleaned up the error message to make it more obvious what is
wrong
```
1685707730734676000 FATAL symbols.sqlite.setup shared/sqlite.go:64 github.com/sourcegraph/sourcegraph/cmd/symbols/shared.SetupSqlite failed to create parser pool {"Resource": {"service.name": "sourcegraph-backend-x86_64-apple-darwin", "service.version": "2023.0.0+dev", "service.instance.id": "Williams-MacBook-Pro"}, "Attributes": {"error": "failed to create new ctags parser \"scip-ctags\" using bin path \"\" : exec: \"universal-ctags\": executable file not found in $PATH"}}
```
## Test plan
1. Compile for Intel Mac `PLATFORM=x86_64-apple-darwin
CROSS_COMPILE_X86_64_MACOS=1 ./enterprise/dev/app/build-backend.sh &&
.bin/sourcegraph-backend-x86_64-apple-darwin`
2. Check that the backend starts up and doesn't crash with the above
error
<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->
- Write Rust one-for-one protocol compatible replacement for
universal-ctags in JSON streaming mode (scip-ctags)
- Use tree-sitter to generate scip symbols, then emit those through
scip-ctags
- These symbols will be reused for Cody context
- Ensure code is built with musl libc
## Test plan
Unit and snapshot tests in the Rust symbol generation code - verified
working in the symbols sidebar and symbol search for enabled languages.
---------
Co-authored-by: TJ DeVries <devries.timothyj@gmail.com>
Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
Co-authored-by: Eric Fritz <eric@eric-fritz.com>
Co-authored-by: Eric Fritz <eric@sourcegraph.com>
Co-authored-by: Jean-Hadrien Chabran <jh@chabran.fr>
- Write Rust one-for-one protocol compatible replacement for
universal-ctags in JSON streaming mode (scip-ctags)
- Use tree-sitter to generate scip symbols, then emit those through
scip-ctags
- These symbols will be reused for Cody context
Currently, only zig is enabled (so other languages should remain unaffected by this change).
We will add other languages throughout the next week as we're able to check them off.
## Test plan
Unit and snapshot tests in the Rust symbol generation code - verified
working in the symbols sidebar and symbol search for enabled languages.
---------
Co-authored-by: SuperAuguste <19855629+SuperAuguste@users.noreply.github.com>
Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
Co-authored-by: Eric Fritz <eric@eric-fritz.com>
Co-authored-by: Eric Fritz <eric@sourcegraph.com>
Context: I am working towards us having two definitions in our codebase:
1. "App" -> the _deployment type_ of Sourcegraph you use on your local
machine.
2. "Single binary" -> App is always single-binary, but not all
single-binaries are App. In the future, all Sourcegraph services will be
"single binary" and so this term will exist in our codebase to refer to
code that is used when ran in a "single binary" setting.
---
Stacked on top of #49064
This change introduces `deploy.IsApp()` and `deploy.IsSingleBinary()`
helpers that do not need to be fed the deployment type.
## Test plan
Existing tests
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Adds forbidigo as a linter than can be executed by nogo.
### What are these other BUILD files for?
The build didn't work without them so had to add them. They were auto
generated by `bazel run //:gazelle`
### Example of errors reported by this linter
```
external/com_github_prometheus_common/model/time.go:164:10: use of `fmt.Errorf` forbidden by pattern `^fmt\.Errorf$` (forbidigo)
external/com_github_prometheus_common/model/time.go:196:13: use of `fmt.Errorf` forbidden by pattern `^fmt\.Errorf$` (forbidigo)
external/com_github_prometheus_common/model/time.go:200:13: use of `fmt.Errorf` forbidden by pattern `^fmt\.Errorf$` (forbidigo)
external/com_github_prometheus_common/model/value.go:53:10: use of `fmt.Errorf` forbidden by pattern `^fmt\.Errorf$` (forbidigo)
external/com_github_prometheus_common/model/value.go:279:10: use of `fmt.Errorf` forbidden by pattern `^fmt\.Errorf$` (forbidigo)
external/com_github_prometheus_common/model/value.go:327:10: use of `fmt.Errorf` forbidden by pattern `^fmt\.Errorf$` (forbidigo)
[609 / 1,838] 10 actions running
GoCompilePkg external/com_github_yuin_goldmark_emoji/definition/definition.a; 3s darwin-sandbox
GoCompilePkg external/com_github_go_enry_go_enry_v2/data/data.a; 3s darwin-sandbox
ERROR: build interrupted
INFO: Elapsed time: 10.957s, Critical Path: 4.42s
INFO: 403 processes: 101 internal, 302 darwin-sandbox.
FAILED: Build did NOT complete successfully
```
### Why do we have our own runAnalysis
Annoyingly, the `analyzer.NewAnalyzer` from forbidigo does not allow you
to configure it in anyway, and we want to add our patterns into it. So
`runAnalysis` was copied from `forbidigo` and slightly modified so that
we can specify patterns
## Test plan
`bazel build //cmd/frontend/...`
<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->
Prior to this change, if you ran Sourcegraph App without Docker
installed or running then when you ran a search request the backend
would make numerous requests to the `symbols` service, which would then
spew an _endless_ stream of errors to the console complaining that
`docker run` failed as the App `CTAGS_COMMAND` tried to run ctags via
Docker.
**I am actively working on bundling/building ctags into the Go binary**
so that it will always be present and this will not be an issue,
however, I need more time to fix that proper and right now this issue is
*severe* and blocks us from showing App to more people:
https://github.com/sourcegraph/sourcegraph/issues/48766
This change improves the current status quo substantially by adding a
few `if isSingleProgram` conditions to simply disable `symbols` (have it
return no results) if you are running App and ctags cannot be started
via Docker. An easy to read message is also shown to the user to tell
them Docker is missing and how it would make Sourcegraph better for
them:
<img width="1163" alt="image"
src="https://user-images.githubusercontent.com/3173176/223233097-f184c4cd-c7a6-47bc-a437-bb301ef946f8.png">
Fixes#48766
Helps #47992
## Test plan
* Ran `sg start app` both with and without Docker running on my system,
verified behavior of both.
* All changes are gated behind a `if isSingleProgram` conditional and so
very low risk to other deployments.
---------
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
* internal: add service and singleprogram packages
* sg.config.yaml: add single-binary build targets
* internal/env: add a function for clearing environ cache
* internal/{workerutil,metrics}: add a hack to allow running 2 executors in the same process
* internal/conf: add single-program deploy type
* internal/singleprogram: clarify security
* cmd/sourcegraph-oss: add initial single-binary main (will not build yet)
* enterprise/cmd/sourcegraph: initial enterprise single-binary
* Add multi-platform builds for single-program
* single-binary: correctly build JS artifacts into binary
* license_finder licenses add github.com/xi2/xz "Public domain"
* internal/service/svcmain: correctly initialize logger for DeprecatedSingleServiceMain
* worker: refactor to new service pattern
* cmd/github-proxy: refactor to use new service pattern
* symbols: refactor to use new service pattern
* gitserver: refactor to user new service pattern
* searcher: refactor to use new service pattern
* gitserver: refactor to use new service pattern
* repo-updater: refactor to use new service pattern
* frontend: refactor to use new service pattern
* executor: refactor to use new service pattern
* internal/symbols: use new LoadConfig pattern
* precise-code-intel-worker: refactor to use new service pattern
* internal/symbols: load config for tests
* cmd/repo-updater: remove LoadConfig approach
* cmd/symbols: workaround env var conflict with searcher
* executor: internal: add workaround to allow running 2 instances in same process
* executors: add EXECUTOR_QUEUE_DISABLE_ACCESS_TOKEN for single-binary and dev deployments only
* single-binary: use EXECUTOR_QUEUE_DISABLE_ACCESS_TOKEN
* extsvc/github: fix default value for single-program deploy type
* single-binary: stop relying on a local ctags image
* single-binary: use unix sockets for postgres
* release App snapshots in CI when pushed to app/release-snapshot branch
* internal/service/svcmain: update TODO comment
* executor: correct DEPLOY_TYPE check
* dev/check: allow single-binary to import dbconn
* executor: remove accidental reliance on dbconn package
* executor: improve error logging when running commands (#46546)
* executor: improve error logging when running commands
* executor: do not attempt std config validation running e.g. install cmd
* executor: do not pull in the conf package / frontend reliance
* ci: executors: correct site config for passwordless auth
* server: fix bug where github-proxy would try to be a conf server
* CI: executors: fix integration test passwordless auth
* executors: allow passwordless auth in sourcegraph/server for testing
* repo-updater: fix enterprise init (caused regression in repository syncing)
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Co-authored-by: Peter Guy <peter.guy@sourcegraph.com>
Co-authored-by: Quinn Slack <quinn@slack.org>
A repo label is set on httptrace metrics, but it only tracks a small set of hardcoded repositories that have not been changed for at least 4 years. There do not appear to be any references to this label in any dashboard either.
origin is another label untouched for 4+ years, and is always set to 'unknown' in s2. Tracking references also reveals two metrics that have no references in dashboards.
Swap out the OpenTracing HTTP middleware with otelhttp, with a default constructor provided in a new package internal/instrumentation. Also adds deprecation notices on internal/trace/ot, and updates deprecation notices in internal/trace so that they render properly in VS Code.
Now that we export to Jaeger via OpenTelemetry instead of OpenTracing, we can remove a number of OpenTracing-specific things internally and instead operate purely on OpenTelemetry within internal/trace. OpenTracing calls elsewhere are all translated to OpenTelemetry via the OpenTracing bridge.
This allows us to start leveraging OpenTelemetry APIs more extensively - the codebase can now directly use go.opentelemetry.io/otel/trace, and internal/trace has been updated with the addition of:
- (*internal/trace.Trace).AddEvent - this is similar to LogFields, but now accepts a name in the OpenTelemetry spec
- (*internal/trace.Trace).SetAttributes - this is similar to TagFields, but uses the OpenTelemetry attributes API
And the deprecation of:
- (*internal/trace.Trace).LogFields (use AddEvent instead)
- (*internal/trace.Trace).TagFields (use SetAttributes instead)
As well as the removal of some old adapters that were specific to OpenTracing. Deprecated APIs will still work, passing through to the updated APIs.
Collection of various stuff I picked up while diving into the code more. Needs a serious cleanup and some more work before it could land, most importantly, symbols would need a main DB connection.
This adds a DB connection to be used for the gitserver client in the symbols service. A follow-up PR will make better use of it. We can also use the DB for some optimizations in the future.
conf.client is typically started with a goroutine that polls the frontend API for configuration updates. If an update is detected, conf.store is updated with a cached value of the new configuration. Consumers of configuration get values from conf.store instead of the frontend API
In the frontend itself, however, we start a conf.Server and a conf.client to be backed by a ConfigurationSource, and both poll this ConfigurationSource for updates. Both hold references to the same conf.store, and both write the updates they receive from ConfigurationSource to the same conf.store. The key difference is that conf.client will notify subscribers of conf.Watch, and conf.Server will not. This leads to the following scenario:
1. Edit the site config in the UI
2. GraphQL API calls conf.Server.Write
3. conf.Server has a channel internally that immediately procs its source poller, which updates conf.store with the new configuration
4. conf.client polls ConfigurationSource and gets the new update. However, it then compares it to conf.store, which has the new value from conf.Server, and detects no diff, causing watchers to not get notified.
This fix:
- removes conf.Server's polling routine entirely
- removes conf.Server's access to conf.store entirely
- to preserve the fast-update behaviour (docstrings indicate this is important for tests), conf.client can now have a channel set, sourceUpdates, that also procs its polling if non-nil
- adds some more robust logging about conf.client updates
Co-authored-by: Keegan Carruthers-Smith <keegan.csmith@gmail.com>
