This PR makes `WithPrefix` visible on the `KeyValue` interface.
Previously, we had other interface implementations that did not support
`WithPrefix`, but now `redisKeyValue` is the only implementation.
`WithPrefix` is currently just used in tests, but it's broadly useful
and will help clean up a bunch of places that wrap Redis and manually
add key prefixes.
We want to discourage direct usage of the Redis pool in favor of routing
all calls through the main `KeyValue` interface. This PR removes several
usages of `KeyValue.Pool`. To do so, it adds "PING" and "MGET" to the
`KeyValue` interface.
This PR removes the `redispool.RedisKeyValue` constructor in favor of
the `New...KeyValue` methods, which do not take a pool directly. This
way callers won't create a `Pool` reference, allowing us to track all
direct pool usage through `KeyValue.Pool()`.
This also simplifies a few things:
* Tests now use `NewTestKeyValue` instead of dialing up localhost
directly
* We can remove duplicated Redis connection logic in Cody Gateway
The `DeleteAllKeysWithPrefix` method is now only used in tests to ensure
the test keyspace is clear. This PR makes it clear this method is only
used in tests, and simplifies the implementation so it no longer needs a
script + direct Redis connection.
Another tiny Redis refactor to prepare for multi-tenancy work.
We have been using v2 data since >5 years now, this should be safe to
remove.
As a side-effect, we have one less background task running in frontend,
which means it ran N times in horizontally scaled environments, which
isn't exactly useful.
Test plan:
Code review.
Recently, this was refactored to also allow using the redispool.Store.
However, that makes it very implicit to know where something is being
written, so instead we pass down the pool instance at instantiation.
This also gives a slightly better overview of where redispool is
actually required.
Test plan: CI passes.
Part of CORE-99
This PR scaffolds the database schema and code structure based on
[CORE-99
comment](https://linear.app/sourcegraph/issue/CORE-99/enterprise-portal-design-sams-user-to-subscription-rpcs#comment-8105ac31)
with some modifications. See inline comments for more elaborations.
- It uses GORM's ONLY for auto migration, just to kick things off, we
may migrate to file-based migration like we are planning for SAMS.
- It then uses the `*pgxpool.Pool` as the DB interface for executing
business logic queries.
Additionally, refactored `subscriptionsservice/v1.go` to use a `Store`
that provide single interface for accessing data(base), as we have been
doing in SAMS and SSC.
## Test plan
Enterprise Portal starts locally, and database is initialized:
* Adding changes to redis caching to be able to support redis store to be able to add more durability to the redis connections
* Adding changes to redis caching to be able to support redis store to be able to add more durability to the redis connections
* Adding changes to redis caching to be able to support redis store to be able to add more durability to the redis connections
* Adding changes to redis caching to be able to support redis store to be able to add more durability to the redis connections
* First pass at moving moving logging linting into Bazel
* fixed negation operators
* Update dev/linters/logging/logging.go
Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
* added more exceptions and refactored one or two impls
* added nogo lint pragmas to offending files
* ran configure
* reverted git-combine refactor
* ran configure
* reverted test as well
---------
Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
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.
* rate limit config job
* fixup comments
* cleanup
* add network to test
* bazel
* address some PR feedback
* bazel
* addressing PR comments
* bazel
* fix const
* address PR comments
* moving the handler out
* addressing PR comments
* fix double call
* initialize handler work like other periodic routines
* remove unused error
* address pr comments and general cleanups
* just use the logger not the full obsContext
* typo+bazel
* match up the default case to current behavior
* leep default behaviorthe same for existing rate limiters
* bazel
This commit contains the initial simple implementation of returning all
recorded Git commands for a given repo without pagination.
Test plan:
GraphQL tests added.
Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com>
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 was missed when implementing the initial rate limiter, we cannot
use NX on the EXPIRE operation, because that's a 7.0+ operator only.
Sourcegraph ships with redis 5 by default, so that will fail on dotcom.
Keegan and I brainstormed this approach together and concluded that we
don't need the MULTI statement and do all in one go, so we can simply
read the TTL and set expiry when it's not yet set.
This is needed to work around tricky we have around things being
declared at init() time, which is fairly common with redis usages in our
codebase.
Also removed the no longer needed backwards.go.
Test Plan: go test
Was hoping we could nicely avoid making the KeyValue interface even
larger, but no dice. Lets do this and see how we can make it not such a
large thing. Additionally the test for KeyValue has become quite large,
but I believe is still readable and debuggable. But there is scope to
improve that in the future as well.
Test Plan: added unit tests. Existing ones test rcache methods changed
as well.
This is something that I thought could be improved. In everycase we
setup a watch for a FIFOList all we are doing it fetching a value.
Instead of updating the FIFOList, we can pass in a function which
FIFOList calls when it wants to know what size to trim to. This
simplifies nearly all use cases and removes the need to have a
SetMaxSize API.
The biggest change here was around syncjobs which had a few mocks setup
which meant the change wasn't as clean. But all other call sites should
look nicer.
Test Plan: go test
Additionally we update KeyValue to correctly model how redis returns
lists. Lists are only returned from list like calls, so we introduce a
Values type to prevent misuse. We also expand the KeyValue interface to
accomodate a new use of HGetAll.
Test Plan: unit tests
FIFOList directly supports the use case of what recorder wants (store
the most recent N jobs). This makes it possible to remove the list
operations added to rcache. Additionally I think the call sites are
clearer.
Test Plan: go test
This is the same as using a value of zero. This is fixes a potential
footgun when accidently passing in a negative value. Additionally
another PR wants to use negative values as a signal to disable a
feature.
Test Plan: added a test
Minor optimization, we would acquire a connection from the pool twice
for Set with TTL since Set would acquire a connection but then just call
SetWithTTL which does the same thing.
Test Plan: go test
KeyValue is an abstraction over our most common uses of redis in our
codebase. It is introduced in an effort to have an alternative to redis
in Sourcegraph App. Additionally it provides a more ergonomic API than
the raw redis interface we have in our codebase.
The most important change is in keyvalue.go. Otherwise the changes to
all other files are much more minimal and should be backwards
compatible.
Note: Some call sites would do multiple commands on a single connection.
The only benefit here is performance, but in each case we did that it
was a function rarely called. Additionally in some places we used
redis.Conn.Send, but now we use redis.Conn.Do always. This means
previously if the command failed we wouldn't know since Send only places
the command into a buffer, and in all cases we ignored the error
returned from Close. Now we won't ignore that error.
Note: Currently Pool should always return, optionally disabling redis is
for a future commit.
Test Plan: unit tests, manual testing and will also do integration tests
in CI.
- Add data access layer for Redis HASHes and LISTs (with tests) (in `rcache.go`)
- Add types on the back end
- Add business logic between GraphQL and Redis
- Add GraphQL endpoint in `schema.graphql` with Node compatibility
- Hook into periodic goroutines, DB-backed workers, and Batches scheduler
- Add menu item, routing, and page with page description, legend at the top, job and routine list
- Add filtering for errors and pause/play
- Extracted `formatMilliseconds` for reuse
- Format tooltip for large numbers nicely
- Remove a cyclic dependency in the `types` package
Co-authored-by: Eric Fritz <eric@sourcegraph.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
This is introduced to fix a rare race condition were we overwrite the
variable slowRequestRedisFIFOList while it could be read. Additionally,
if this store is not backed in memory we want to re-use the in memory
store.
Test Plan: go test
This essentially disables the FIFOList. The outbound request logger has
this as a possibility which I intend to port to FIFOList.
Note: we have to special case maxSize 0, otherwise the LTRIM we send is
"LTRIM key 0 -1" which means don't trim anything.
Test Plan: go test
* [fix] only send unlock account email once per locked session
* Revert print statements from unrelated debugging
* PR feedback
* Fix bug with SetKeyTTL method
* Update internal/rcache/rcache.go
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
* Set TTL based on what we configure in site config
* Remove global mock and fix unit tests
* Remove innefective use of mat.Min
* Update cmd/frontend/internal/auth/userpasswd/lockout.go
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
* Added Redis access
* Added RedisLogItem with request+response info and stack traces
* Added RedisLoggerMiddleware, with sensitive header removal
* Added GraphQL API endpoint, for site admins only, matching the Cursor Connections spec
* Added admin UI with five-sec polling and "copy curl" feature
* Added back-end tests
* Added log limit setting with default=50, range: 0..500
* Added changelog item
* Deduplicated WebhookLogHeader with HTTPHeader
* Unrelated: Fixed two typos
* Use for the API
* Made the CSS work well with both light and dark themes and made sure it's accessible. Tested with a screen reader.
Co-authored-by: Robert Lin <robert@bobheadxi.dev>
Adds a new permissionsSyncJobs query that returns the most recent outcomes of permissions sync jobs, with per-provider details included (see #44258). This will be used to validate permissions syncing is starting correctly and the desired providers are being involved and in a healthy state.
We don't implement pagination since the endpoint is intended as a stream of recent events and retention is low, but the query schema is set up so that it can be extended to support pagination in the future.
Co-authored-by: Erik Seliger <erikseliger@me.com>
Tracks perms sync job outcomes, notably per-provider states (#44257), in Redis. We set an aggressive TTL of 5 minutes by default because in practice, only the details for the last few sync jobs are relevant, and this prevents us from putting too much pressure on Redis on larger instances.
This can be extended in the future to be database-backed, but for now I think Redis is the right choice - the volume of entries can be quite high when accumulated so we want something easy to expire, and realistically only the details for the last few sync jobs are relevant. Additionally, the records store and reader in package synclogs can easily be rewritten without too much impact to write to a database in the future.
The redigo/redis version we use now was published in 2018, and has since been retracted - the latest versions are still being published on 1.x, with the latest release v1.8.9 being published this year:
https://pkg.go.dev/github.com/gomodule/redigo@v1.8.9/redis?tab=versions
This upgrades our dependency to use this latest version, and also removes an unused rcache mutex functionality from package rcache that no longer works with this new version of redigo.
