-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support definitions from JavaCC files #39
Labels
feature-request
stale
team/graph
Graph Team (previously Code Intel/Language Tools/Language Platform)
Milestone
Comments
sqs
added
the
team/graph
Graph Team (previously Code Intel/Language Tools/Language Platform)
label
Oct 25, 2018
chrismwendt
added a commit
that referenced
this issue
Nov 4, 2018
Please post an update or close this with an explanation if it is no longer relevant. |
Stalebot is closing this due to inactivity. |
41 tasks
BolajiOlajide
added a commit
that referenced
this issue
Mar 13, 2023
This PR adds a logger for events, so rbac-related events can be viewed in the `event_logs` table. I also removed the constraint where a site admin can't assign a role to themselves. ## Test plan <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> * Manual testing * Update unit tests. <br> Backport 0ac73dd from #49181 Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com>
vovakulikov
added a commit
that referenced
this issue
Mar 13, 2023
… result and blob pages) (#49261) Addresses search UI padding problem ([slack discussion](https://sourcegraph.slack.com/archives/C03CSAER9LK/p1678548618527699?thread_ts=1678361542.292339&cid=C03CSAER9LK)) | | Before | After | | ----- | -------- | -------- | | Initial state | <img width="1113" alt="Screenshot 2023-03-11 at 22 51 05" src="https://user-images.githubusercontent.com/18492575/224519899-60e75361-dea9-425c-891f-ca7efbfaeb68.png"> | <img width="1113" alt="Screenshot 2023-03-11 at 22 51 03" src="https://user-images.githubusercontent.com/18492575/224519888-1df65561-4540-4b01-82dd-33436e126e6c.png"> | | Focused state | <img width="1113" alt="Screenshot 2023-03-11 at 22 51 07" src="https://user-images.githubusercontent.com/18492575/224519879-083605bc-7286-45f3-bcb3-64fefcd4e2cd.png"> | <img width="1113" alt="Screenshot 2023-03-11 at 22 50 42" src="https://user-images.githubusercontent.com/18492575/224519864-54a34cbf-3899-4f7c-91e2-97fc6ae6f286.png"> | ## Notes - Home page search input isn't affected by this change (it still uses standard UI mode) - @rrhyne I implemented this in the way you suggested in the thread (added a thin border around input in focus state), but let me know if you want to try the no around-border approach, No borders-approach in the image below <img width="600" alt="Screenshot 2023-03-11 at 22 57 50" src="https://user-images.githubusercontent.com/18492575/224519986-756f897d-3266-4d09-82b3-aa75b8f0c8a2.png"> - @rrhyne, I'm not sure about the padding in the suggestion panel, I tried to keep alignment with the history icon buttons, but it may feels like it's too close to the borders in the compact mode. Let me know if we can ignore alignment with history icon and make the padding bigger there. - @fkling I re-layout the previous version and tried to simplify styles and HTML in the search box UI, I didn't figure out why we had to use resize observer there if this input doesn't support multiple (and always has the same height value) but let me know if there is a case for this, I think we could bring back resize observer and don't use hardcoded values for padding in CSS) ## Test plan - Check the Home page old/new search UI - Check the search result page old/new search UI - Check file page old/new search UI Co-authored-by: Vova Kulikov <vovakulikov@icloud.com>
eseliger
pushed a commit
that referenced
this issue
Mar 14, 2023
olafurpg
added a commit
that referenced
this issue
Mar 14, 2023
…9309) Previously, we had to manually override each language for new tree-sitter supported language. This was error prone and resulted in Ruby and Python not having tree-sitter highlighting enabled by default. With this change, there's one less place we have to update when we add a new tree-sitter highlighter. ## Test plan Manually opened all files in this directory and verified they are using tree-sitter highlighting https://sourcegraph.test:3443/github.com/sourcegraph/sourcegraph/-/blob/docker-images/syntax-highlighter/crates/sg-syntax/src/snapshots/files/typescript.ts <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport 386ff31 from #49112 Co-authored-by: Ólafur Páll Geirsson <olafurpg@gmail.com>
thenamankumar
added a commit
that referenced
this issue
Mar 14, 2023
closes: https://github.com/sourcegraph/sourcegraph/issues/49168 closes: https://github.com/sourcegraph/sourcegraph/issues/49285 - Allow non admin users to view their permissions sync jobs - Allow site admins on dotcom to view user's email via `User` gql type. They already can view that from `SiteUser` gql type used in user management. ## Test plan - added unit tests <br> Backport a3b5e62 from #49246 Co-authored-by: Naman Kumar <naman@outlook.in>
vovakulikov
added a commit
that referenced
this issue
Mar 14, 2023
We removed this flag before but apparently it's still with us, I think there was a bad merge and we haven't removed it completely from the code base, this PR does this ## Test plan - Make sure that CI is green <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport ac47e7c from #49334 Co-authored-by: Vova Kulikov <vovakulikov@icloud.com>
olafurpg
added a commit
that referenced
this issue
Mar 15, 2023
Several small improvements to the C++ highlighting queries to improve readability. - Treat destructors as functions, same as constructors - Treat `unsigned` and `signed` as builtin types, same as primitive types like `bool` - Treat all-uppercase variables as identifiers instead of constants - Treat function declarations with field names as functions instead of fields - Treat `operator` in `operator==` as a keyword - Remove usages of `IdentifierOperator` because it's noisy, doesn't improve readability of the code and there were many false positives in the implementation - Include `~` in destructor names - Handle C++ attributes - Handle C++ literal suffixes - Adds new minimized test case for various language features ## Test plan See updated snapshot tests. <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport 93a1101 from #49254 Co-authored-by: Ólafur Páll Geirsson <olafurpg@gmail.com>
olafurpg
added a commit
that referenced
this issue
Mar 15, 2023
* Treat raw string literals as strings * Treat nullptr as a null literal I was hoping to be able to use `@string.escape` for the character sequence before in raw string literals but the grammar we're using (official C++ grammar) doesn't provide AST nodes for them. We should consider moving to a non-official grammar that supports these, see https://sourcegraph.slack.com/archives/C03C5EFRMR8/p1678881469547489 ## Test plan See updated snapshots. <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport af63a4c from #49404 Co-authored-by: Ólafur Páll Geirsson <olafurpg@gmail.com>
vovakulikov
pushed a commit
that referenced
this issue
Mar 15, 2023
Prior to this PR right after you finish setup flow you still can see global alert about intense setup. In setup wizard we have a logic about siteFlags reloading but it didn't work properly because site flags query had cache-first policy We simply change it and with cache-and-network policy refreshSiteFlags query works as expected ## Test plan - Open setup wizard when you have 0 connected code hosts - Finish setup wizard - You should see no global alerts about setup experience
fkling
added a commit
that referenced
this issue
Mar 15, 2023
…t selection (#49378) **NOTE:** Hide whitespace changes for review This commit avoids highlighting tokens and showing hover tooltips during text selection by - ignoring mouse events when a button is pressed - checking whether text is selected and intersection with the hovered token Even if the user is not actively selecting text, text selection and token highlighting a somewhat at odds with each other (I think text selection and _dynamic_ background colors don't work well together). So with this update no hover information will be shown for tokens that are intersection the text selection (hover information is still shown for tokens outside the text selection). The biggest change in the implementation is that the tooltip extension and the token decoration extension both use the `tooltipInformationFacet` facet to determine what to show instead computing the tooltip information based on their own positioning information. ## Test plan - Enter query. - Hover over tokens -> token highlighted and tooltip shown - Slowly select text -> no token highlighting or tooltips - Hover over tokens outside of text selection -> token highlighted and tooltip shown - Hover over tokens intersecting with text selection -> no token highlighting or tooltips - Place cursor at the end of a token and hover hover token -> token highlighted and tooltip shown - Place cursor at the beginning of a token and hover over token -> token highlighted and tooltip shown Co-authored-by: Felix Kling <felix@felix-kling.de>
Strum355
pushed a commit
that referenced
this issue
Mar 16, 2023
To include the fix from sourcegraph/scip-typescript#239 in the 5.0 release. ## Test plan Ran the following command ``` ❯ docker run sourcegraph/scip-typescript@sha256:1851ad42b3b47c8fb240c5060b5757cf51ebeece5e360013e41ab8a1dd05d52c scip-typescript --version WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested 0.3.7 ``` <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport d7070e3 from #49499 Co-authored-by: Ólafur Páll Geirsson <olafurpg@gmail.com>
vovakulikov
added a commit
that referenced
this issue
Mar 16, 2023
) Fixes https://github.com/sourcegraph/sourcegraph/issues/49517 It seems like with the Apollo version upgrading, we broke code insight polling logic, onCompleted doesn't run on every fetch call in the new version, and we rely on this in the code insight logic. This PR fixes this problem and also tunes the polling interval taking into account repositories query API ## Test plan - Check that polling works properly right after you're created an insight you can see how it updates insight on the dashboard - Check that insight polling is disabled if insight isn't in the visible viewport area - Check that polling enables when insight comes into the visible area - Check that polling works on the standalone insight page <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> Co-authored-by: Vova Kulikov <vovakulikov@icloud.com>
vdavid
pushed a commit
that referenced
this issue
Mar 17, 2023
- **Giving 404 response for invalid IDs**: Okta validator sends stuff like `010101001010101011001010101011` to check nonexistent users. We returned `400` to these, which made the tests fail. We now return `404`. - **Simplified docs**: The `scim.identityProvider` setting was mentioned as required, and its description wasn't that specific and led me to believe that this setting was more than just "standard/Azure AD". I've marked it as optional, and cleared up its description. - **Renamed two constants** to match Go standards. (My IDE gave a warning on these.) - **Deleted unused arguments** to get rid of code warnings. - I also **fixed some typos** in comments. - Tested with Azure AD validator and Okta's SPEC validator. <br> Backport e1f8f7a from #49393
vovakulikov
added a commit
that referenced
this issue
Mar 17, 2023
Reported in [slack](https://sourcegraph.slack.com/archives/C01RVEVPWLC/p1679055905424439) Prior to this PR clear filters button didn't work as it should (it sets previous values for the series display option, for some reason), and as a result, it didn't clear the series display options section in case you had a series display option filter before. ## Test plan - Clear filters button should clear all filters (series display options as well as all other filters) - After you cleared the filters you should see no purple dot next to filter button (because you don't have any filters applied anymore) Co-authored-by: Vova Kulikov <vovakulikov@icloud.com>
cbart
added a commit
that referenced
this issue
Mar 17, 2023
This change introduces a check on `UpdateTeam` to not introduce a circular parent relationship. The check effectively finds all teams that _are not descendants_ (so not in the subtree of) modified team, and looks whether the parent is there. It is only safe to set the parent team ID to a team outside of modified team's own descendants. ## Test plan * Testing database stores with Postgres. * Testing GraphQL API with fakes. <br> Backport da92373 from #49556 Co-authored-by: Cezary Bartoszuk <cezary.bartoszuk@sourcegraph.com>
dadlerj
added a commit
that referenced
this issue
Mar 17, 2023
…ween critical and non-critical telemetry (#49610) Proof of concept PR here. I'm failing in Buildkite and am struggling resolving some of them (I can't even seem to get prettier to run anymore, it's been too long). I would love to work with andersonlauren and ryphil to (1) make specific reductions to our telemetry as part of this or soon after, and (2) get eng support to fully clean up and remove the old config option. ## Test plan * Run a local instance, set disableNonCriticalTelemetry to true * See if any errors appear * Confirm telemetry sent is identical regardless of the value of the configuration option above <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport 0337361 from #48866 Co-authored-by: Dan Adler <dadlerj@users.noreply.github.com>
Strum355
pushed a commit
that referenced
this issue
Mar 17, 2023
…#49630) ## Description This PR: - Adds a "Add filter" button from the "Manage filters" modal - Now supports editing the `versionFilter` `name` value. Previously we just assumed it was readonly, as it was only accessible from a specific package. - Moved summary of both the `versionFilter` and `nameFilter` responses towards the bottom of the modal. I have reworked this slightly as I think I made too many assumptions before that a user would be blocking an _existing_ package, when they could just be blocking a future package. ## Screenshots Managing filters <img width="1022" alt="image" src="https://user-images.githubusercontent.com/9516420/225566512-e7458b48-0416-4cd5-85c2-89da8f7c68c9.png"> Adding a version filter that doesn't match anything yet: <img width="1048" alt="image" src="https://user-images.githubusercontent.com/9516420/225564627-28fb19b5-7085-4e5a-a7c4-a8f96f949172.png"> Adding a name filter that doesn't match anything yet <img width="1014" alt="image" src="https://user-images.githubusercontent.com/9516420/225566280-d91fb4e9-6ebb-4b6e-aa87-d203adfafaa3.png"> Normal behavior: <img width="1076" alt="image" src="https://user-images.githubusercontent.com/9516420/225564922-b51887dc-b88b-4c60-9968-bbffadc26f25.png"> <img width="1060" alt="image" src="https://user-images.githubusercontent.com/9516420/225565796-31e3709c-2918-4519-9296-2a723ead09b3.png"> ## Test plan Tested locally <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> ## App preview: - [Web](https://sg-web-tr-packages-manage-add-filter.onrender.com/search) Check out the [client app preview documentation](https://docs.sourcegraph.com/dev/how-to/client_pr_previews) to learn more. <br> Backport 47c52c0 from #49493 Co-authored-by: Tom Ross <tom@umpox.com>
eseliger
pushed a commit
that referenced
this issue
Mar 18, 2023
This PR is composed of three commits: 1) Skip running code monitors if the user is deleted. This will always fail, and will end up in a retry loop. 2) Stop running all code monitors in a transaction. In particular, when the transaction is rolled back, the "next run" time is unset, which puts us in another retry loop. 3) Remove transitional code. The feature flag has been enabled by default for quite a while now, and there are no plans to disable it. Follow up from [this thread](https://sourcegraph.slack.com/archives/CHEKCRWKV/p1678322373363769). This does not fix the problem that invalid queries will always error, but it should ensure they don't run in a hot retry loop.
eseliger
pushed a commit
that referenced
this issue
Mar 20, 2023
This image is not meant for production use. It enables the use of some very basic batch changes when using the native-SSBC feature flag, but is not isolated and thus should not be used. More context on why we're doing this: https://sourcegraph.slack.com/archives/C07KZF47K/p1678954730651559?thread_ts=1678480841.394689&cid=C07KZF47K We should undo this change once we're in a better situation for the reason of this PR.
coury-clark
pushed a commit
that referenced
this issue
Mar 21, 2023
- Completes https://github.com/sourcegraph/sourcegraph/issues/49216 - Completes https://github.com/sourcegraph/sourcegraph/issues/49020 Three changes: - Added Okta-specific setup info for admins - Added dev docs to capture the most crucial info that we only had in the SCIM Google Doc - Linked SCIM from SAML pages for Okta and Azure AD to improve discoverability at the points where it's the most relevant ## Test plan Only docs changes. I've clicked through the links. <br> Backport 2b60998 from #49678 Co-authored-by: David Veszelovszki <veszelovszki@gmail.com>
BolajiOlajide
pushed a commit
that referenced
this issue
Jul 11, 2023
…guration (#54555) Closes #52901 Earlier, when we surfaced the rollout window config for non-site admins, we didn't test this feature with a non-site-admin account; this led to non-site admins seeing the error below. ![image](https://github.com/sourcegraph/sourcegraph/assets/25608335/08018eea-245e-4b5c-ac5a-27fd7a3e0812) This is expected because the graphql query shown below, which we use to fetch the rollout window configuration, only allows admins to fetch the config. ```graphql query { site { configuration { ... } } } ``` This PR adds an argument to the `configuration` field, which will return a list of whitelisted configs that can be displayed to the user. ## Test plan <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> * Configure rollout windows on your Sourcegraph instance `batchChanges.rolloutWindows` as a site-admin. * Log in as a non-site-admin * Access the Batch Changes User settings `/users/bool/settings/batch-changes` * You should be able to view the rollout window configured. <img width="1154" alt="CleanShot 2023-06-27 at 14 45 10@2x" src="https://github.com/sourcegraph/sourcegraph/assets/25608335/2de0387a-a3ef-43b9-a937-5fad49a98cd0"> <br> Backport 0b6bc0b from #54242
coury-clark
pushed a commit
that referenced
this issue
Jul 11, 2023
…n on read errors (#54804) When a set of repositories are to be scheduled for embeddings (via graphql or a policy) an error with a single repository can cause the entire set of embeddings jobs to be rolled back. The specific error that occurred for a recent customer issue ([slack discussion](https://sourcegraph.slack.com/archives/C053L1AQ0BC/p1688671109829779)) was due to one of the [repos being empty](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/embeddings/schedule.go?L36-42) and causing all 500 repos matched by a policy to not be enqueued for embeddings jobs. Not only do all repos have their embeddings jobs rolled back but additionally the logging will only report one problematic repo before exiting the scheduling logic which makes it so that the log does not capture all of the problematic repos in one attempt. We should instead allow read errors (e.g. reading sql or fetching information from gitserver) to be handled but not prevent the entire batch of jobs to be rolled back. This will allow a policy to be partially impactful across the repositories it is concerned with. Especially given that an empty repo won't have any need to be enqueued for an embeddings job. This PR changes the error handling so that repos that cannot complete an embeddings job will still be enqueued with a `failed` state so that site admin pages can still display which individual repos are problematic. When a repo is enqueued with a `failed` state we do not roll back the transaction so that successful embeddings jobs are still created with a `queued` state. ## Test plan will add test case to either scheduling logic tests or job execution logic tests (depends on implementation) <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport 57146ce from #54701 Co-authored-by: Gary Lee <105310278+gl-srgr@users.noreply.github.com>
camdencheek
pushed a commit
that referenced
this issue
Jul 17, 2023
…t UI (#55000) This PR removes the restrictions of having one Auth provider per URL, and instead allows one auth provider for each URL+ClientID combination. The URL restriction came from the way our old permissions table worked, but with the `user_repo_permissions` table, this restriction no longer applies as permissions are stored against individual external accounts. This has also become an increasingly common ask from customers, and it also eases the transitioning when switching from one auth provider to another, like switching from traditional GitHub OAuth to GitHub App Oauth. One caveat that remains: Having multiple auth providers for the same URL does not play well with repo-centric permissions syncing, as we cannot from the repo token's perspective determine which user external account a permission should be bound to. There isn't a clean workaround to make this work, and the suggestion is to turn off repo-centric permissions syncing when using multiple auth providers to the same URL. The idea is to have a follow-up PR that allows us to disable repo-centric permissions syncs for individual code host connections.
BolajiOlajide
pushed a commit
that referenced
this issue
Jul 19, 2023
Closes #54702 I'm still trying to figure out when this started happening or why it started happening. However, according to the error message it looks like there's an invalid character within the character class (usually denoted by the [] brackets) ![image](https://github.com/sourcegraph/sourcegraph/assets/25608335/00833274-d919-408a-959a-c46735cb8b4c) In the given regular expression `/^\w(?:\w|[.-](?=\w))*-?$/`, the invalid character class is [.-]. Escaping the hyphen character within the character class `[.\-]` fixes the error. ## Test plan <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> Spin up a fresh instance of Sourcegraph, entering the username and password shouldn't trigger the error in the console anymore. ![CleanShot 2023-07-07 at 12 16 25@2x](https://github.com/sourcegraph/sourcegraph/assets/25608335/f621a1ee-b320-4d92-95f8-7d46a033e6c4) <br> Backport 3e7ee5f from #54708
coury-clark
pushed a commit
that referenced
this issue
Jul 24, 2023
…55160) This should reduce memory usage significantly, as in the common case (no two documents share the same relative path), we will end up processing one document at a time. I've tested the new code with a 5.7GB Chromium index, and we're able to process it with even 100MB of memory (at the cost of increased GC pressure). We need to iterate over the index twice, first to get all external symbols, and then to process documents, as document processing requires access to the external symbols list. This means we need the ability to seek to the start again. I've implemented that as follows: - For small indexes, just read the index into a slice. - For large indexes, save the compressed index to a temporary file on disk, and rely on the GC and page cache to transparently drop pages earlier in the file when under memory pressure. I figured decompressing is cheap enough, that it doesn't make sense to have the extra I/O overhead of reading/writing the uncompressed index. Other changes: - Documents are no longer sorted by relative path during iteration The order of iteration is still deterministic though as it matches the order of documents in the index. Questions: - Should we add instrumentation see the memory usage at different stages of processing an index? - How do we add a memory usage test (either here or in the upstream scip bindings)? ([internal Slack discussion](https://sourcegraph.slack.com/archives/C3B3SDBMY/p1687751363185919)) ## Test plan - [x] Update existing tests - [x] Add low memory usage test -- added upstream. sourcegraph/scip#181 <br> Backport b98ca76 from #53828 Co-authored-by: Varun Gandhi <varun.gandhi@sourcegraph.com>
coury-clark
pushed a commit
that referenced
this issue
Jul 24, 2023
…t for all error types (if enabled) (#55232) ## overview This PR tweaks the internal error interceptor implementation to be able to print the initial request message for _all_ internal error types, not just utf-8 errors. This feature is still off by default for security / privacy reasons. It can be enabled via the `SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_PROTOBUF_MESSAGES_ENABLED` environment variable. ## notes ### size limits If the protobuf message that we're trying to log is quite large (hundreds of megabytes or gigabyte+), rendering the JSON representation of the message could be quite expensive. (The JSON form of the message is larger than the original protobuf message, so attempting to log a large message could result in excessive memory usage. Ex: a 5 GB proto message could result in a 6 gigabyte JSON string, resulting in 11 GB of total allocations). To solve this, I introduced an environment variable `SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_PROTOBUF_MESSAGES_HANDLING_MAX_MESSAGE_SIZE_BYTES`. If the protobuf message is larger than this (calculated using [`proto.Size`](https://pkg.go.dev/google.golang.org/protobuf/proto#Size)), then it won't be turned into JSON at all. (An error message will be returned instead). ```text [ frontend] ERROR gitserver.client.gRPC.internal.error.reporter.streamingMethod.postMessageReceive internalerrs/logging.go:238 grpc: received message larger than max (105906242 vs. 4194304) {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "ResourceExhausted", "error": "failed to marshal protobuf message (key: \"initialRequestJSON\") to bytes: message too large (size \"106 MB\", limit \"105 MB\")"} ``` The default value is 100 MB. ### environment variable name changes Since we log in more cases then just for non-uft8 string errors now, I tweak the names of the environment variables to reflect that: | before | after | |:-----------------------------------------------------------------------------:|:--------------------------------------------------------------------------------:| | SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_NON_UTF8_PROTOBUF_MESSAGES_ENABLED | SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_PROTOBUF_MESSAGES_ENABLED | | SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_NON_UTF8_PROTOBUF_MESSAGES_MAX_SIZE_BYTES | SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_PROTOBUF_MESSAGES_JSON_TRUNCATION_SIZE_BYTES | (The `SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_PROTOBUF_MESSAGES_HANDLING_MAX_MESSAGE_SIZE_BYTES` environment variable didn't exist before, since the concept of skipping logging a message in the first place because it was too large didn't exist until now). Since I changed the environment variables name, this will mean that that any instances that had the message logging feature enabled will have it disabled. Luckily, this only affects sourcegraph.com. After this PR is merged and lands, I'll adjust sourcegraph.com to change the appropriate environment variable names (in the same way as I did it in this PR sourcegraph/deploy-sourcegraph-cloud#17936) ## Test plan Local testing. I applied the following diff to force a non-utf8 error, ran a search for `context:global test type:diff`, and saw the same logs as I expected before: ```diff diff --git a/cmd/gitserver/server/server_grpc.go b/cmd/gitserver/server/server_grpc.go index 003fe482bc..0307c4fdd8 100644 --- a/cmd/gitserver/server/server_grpc.go +++ b/cmd/gitserver/server/server_grpc.go @@ -310,8 +310,12 @@ func (gs *GRPCServer) Search(req *proto.SearchRequest, ss proto.GitserverService } onMatch := func(match *protocol.CommitMatch) error { + + m := match.ToProto() + m.Oid = strings.Repeat("\xc5", 1*1024*1024) return ss.Send(&proto.SearchResponse{ - Message: &proto.SearchResponse_Match{Match: match.ToProto()}, + + Message: &proto.SearchResponse_Match{Match: m}, }) } ``` ``` [ frontend] ERROR gitserver.client.gRPC.internal.error.reporter.streamingMethod.postMessageReceive internalerrs/logging.go:244 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "Internal", "initialRequestJSON": "{\"repo\":\"github.com/sourcegraph-testing/etcd\",\"revisions\":[{}],\"limit\":50000,\"include_diff\":true,\"query\":{\"Value\":{\"DiffMatches\":{\"expr\":\"test\",\"ignore_case\":true}}}}", "nonUTF8StringFields": [], "messageJSON": "{\"Message\":null}"} [ gitserver-1] ERROR gitserver.gRPC.internal.error.reporter.streamingMethod.postMessageSend internalerrs/logging.go:244 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "Internal", "initialRequestJSON": "{\"repo\":\"github.com/Eltecz/STM32G4_OSC\",\"revisions\":[{}],\"limit\":50000,\"include_diff\":true,\"query\":{\"Value\":{\"DiffMatches\":{\"expr\":\"test\",\"ignore_case\":true}}}}", "nonUTF8StringFields": ["match.oid"], "messageJSON": "{\"Message\":{\"Match\":{\"oid\":\"\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd...(truncated 6294534 bytes)"} ``` In order to test the `SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_PROTOBUF_MESSAGES_HANDLING_MAX_MESSAGE_SIZE_BYTES` feature, I manually set it to 100 bytes and re-ran the same search: ``` [ frontend] ERROR gitserver.client.gRPC.internal.error.reporter.streamingMethod.postMessageReceive internalerrs/logging.go:244 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "Internal", "initialRequestJSON": "{\"repo\":\"github.com/sourcegraph-testing/titan\",\"revisions\":[{}],\"limit\":50000,\"include_diff\":true,\"query\":{\"Value\":{\"DiffMatches\":{\"expr\":\"test\",\"ignore_case\":true}}}}", "nonUTF8StringFields": [], "messageJSON": "{\"Message\":null}"} [ gitserver-1] ERROR gitserver.gRPC.internal.error.reporter.streamingMethod.postMessageSend internalerrs/logging.go:244 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "Internal", "initialRequestJSON": "{\"repo\":\"github.com/sourcegraph-testing/tidb\",\"revisions\":[{}],\"limit\":50000,\"include_diff\":true,\"query\":{\"Value\":{\"DiffMatches\":{\"expr\":\"test\",\"ignore_case\":true}}}}", "nonUTF8StringFields": ["match.oid"], "error": "failed to marshal protobuf message (key: \"messageJSON\") to to string: message too large (size \"1.1 MB\", limit \"100 B\")"} ``` I tested that JSON truncation still worked by setting `SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_PROTOBUF_MESSAGES_JSON_TRUNCATION_SIZE_BYTES` to 100 bytes and re-running the same search: ``` [ gitserver-1] ERROR gitserver.gRPC.internal.error.reporter.streamingMethod.postMessageSend internalerrs/logging.go:244 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "Internal", "initialRequestJSON": "{\"repo\":\"github.com/sourcegraph-testing/titan\",\"revisions\":[{}],\"limit\":50000,\"include_diff\":true,\"q...(truncated 67 bytes)", "nonUTF8StringFields": ["match.oid"], "messageJSON": "{\"Message\":{\"Match\":{\"oid\":\"\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd\\ufffd...(truncated 6291941 bytes)"} [ frontend] ERROR gitserver.client.gRPC.internal.error.reporter.streamingMethod.postMessageReceive internalerrs/logging.go:244 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "Internal", "initialRequestJSON": "{\"repo\":\"github.com/sourcegraph-testing/titan\",\"revisions\":[{}],\"limit\":50000,\"include_diff\":true,\"q...(truncated 67 bytes)", "nonUTF8StringFields": [], "messageJSON": "{\"Message\":null}"} ``` I tested that disabling the feature still worked by setting `SRC_GRPC_INTERNAL_ERROR_LOGGING_LOG_PROTOBUF_MESSAGES_ENABLED` to `false` and re-running the same test: ``` [ gitserver-0] ERROR gitserver.gRPC.internal.error.reporter.streamingMethod.postMessageSend internalerrs/logging.go:244 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "Internal", "nonUTF8StringFields": ["match.oid"]} [ frontend] ERROR gitserver.client.gRPC.internal.error.reporter.streamingMethod.postMessageReceive internalerrs/logging.go:244 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "Internal", "nonUTF8StringFields": []} ``` Lastly, I checked to make sure that the new logic did actually print proto messages for other kinds of internal errors. I applied the following diff to induce a "message size too large" error: ```diff diff --git a/cmd/gitserver/server/server_grpc.go b/cmd/gitserver/server/server_grpc.go index 003fe482bc..5b93dbcac4 100644 --- a/cmd/gitserver/server/server_grpc.go +++ b/cmd/gitserver/server/server_grpc.go @@ -310,8 +310,12 @@ func (gs *GRPCServer) Search(req *proto.SearchRequest, ss proto.GitserverService } onMatch := func(match *protocol.CommitMatch) error { + + m := match.ToProto() + m.Oid = strings.Repeat("a", 100*1024*1024) return ss.Send(&proto.SearchResponse{ - Message: &proto.SearchResponse_Match{Match: match.ToProto()}, + + Message: &proto.SearchResponse_Match{Match: m}, }) } ``` After re-running the same search, I saw the following expected log messages: ``` [ frontend] ERROR gitserver.client.gRPC.internal.error.reporter.streamingMethod.postMessageReceive internalerrs/logging.go:244 grpc: received message larger than max (104858917 vs. 67108864) {"grpcService": "gitserver.v1.GitserverService", "grpcMethod": "Search", "grpcCode": "ResourceExhausted", "initialRequestJSON": "{\"repo\":\"github.com/hashicorp/go-multierror\",\"revisions\":[{}],\"limit\":500,\"include_diff\":true,\"query\":{\"Value\":{\"DiffMatches\":{\"expr\":\"test\",\"ignore_case\":true}}}}"} ``` <br> Backport c6194a2 from #55130 Co-authored-by: Geoffrey Gilmore <geoffrey@sourcegraph.com>
DaedalusG
pushed a commit
that referenced
this issue
Jul 24, 2023
…sed to create GitHub Apps (#55238) Previously, we were using Go's default HTTP Client (`http.DefaultClient`) to invoke GitHub to create GitHub apps. This client does not work with custom certificates that may need to be used if calling an on-prem GitHub instance. ### Changes * Updated to use our `httpcli.UncachedExternalClientFactory` instead of `http.DefaultClient` * This gives us access to logging, tracing, etc... out of the box * Loading certs that are configured in `experimentalFeatures.tls.external.certificates` into the HTTP client * This happpens automagically with `httpcli.UncachedExternalClientFactory`. It has an option called `ExternalTransportOpt` that loads the certs from `tls.external` (see recording to see this in action) ## Test plan Added new tests and tested manually (see below for recordings) ### Creating an app https://github.com/sourcegraph/sourcegraph/assets/38407415/ffb78a7b-43a1-41bf-b22e-b2e9f595122f ### Validation of custom certs being loaded https://github.com/sourcegraph/sourcegraph/assets/38407415/47135e09-1fad-4086-82ce-056d4067e386 <br> Backport 97fad93 from #55084 Co-authored-by: Randell Callahan <piszmogcode@gmail.com>
BolajiOlajide
pushed a commit
that referenced
this issue
Jul 26, 2023
…ing Referer (#55307) Firefox's default settings do not forward Referer headers for privacy reasons. This means that GitHub App setups fail when done using Firefox. This PR adds the GitHub App's base url to the state we store in Redis, so that we no longer rely on the browser sending the Referer header. ## Test plan Tests updated. <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport 02ee893 from #55305
BolajiOlajide
pushed a commit
that referenced
this issue
Jul 26, 2023
…across multiple messages (#55292) This PR changes the LocalCodeIntel symbols gRPC method to return a _stream_ of chunks of symbols, rather than the entire result set in a single message. --- gRPC has default limits on how large individual messages can be. This is because gRPC is a message-based framework, which means that messages have to be entirely loaded into memory for **both** sending and receiving. As such, large allocations can negatively impact server performance and stability. The go-grpc default limit is 4MB. The original LocalCodeIntel RPC implementation (both gRPC and REST) returned the entire set of returned symbols in one message. For certain repositories / files, this message can contains _thousands_ of symbols, which can add up to **[hundreds of MB](https://console.cloud.google.com/logs/query;cursorTimestamp=2023-07-24T15:43:57.509461420Z;duration=P7D;query=resource.type%3D%22k8s_container%22%0Aresource.labels.project_id%3D%22sourcegraph-dev%22%0Aresource.labels.location%3D%22us-central1-f%22%0Aresource.labels.cluster_name%3D%22cloud%22%0Aresource.labels.namespace_name%3D%22prod%22%0AjsonPayload.InstrumentationScope%3D~%22gRPC.internal.error.reporter%22%0A-jsonPayload.message%3D%22grpc:%20error%20while%20marshaling:%20string%20field%20contains%20invalid%20UTF-8%22%0A-jsonPayload.Body%3D%22grpc:%20error%20while%20marshaling:%20string%20field%20contains%20invalid%20UTF-8%22%0Atimestamp%3D%222023-07-24T15:43:57.509461420Z%22%0AinsertId%3D%22f964g54awjrevxzi%22?project=sourcegraph-dev) or even [gigabytes of memory](https://sourcegraph.slack.com/archives/C04HCK4K3DL/p1683228335350079).** This PR adjusts the server-side implementation of LocalCodeIntel to chunk up its full result set into smaller chunks (so that each individual gRPC message is smaller). It does this in a few steps. - Adopting a `chunk` utility I found in the [Gitaly](https://gitlab.com/gitlab-org/gitaly/-/blob/v16.2.0/internal/helper/chunk/chunker.go) project that can intelligently send a group of protobuf messages in smaller chunks (all ~ 1MB): sourcegraph/sourcegraph@59ba6d0 - I made some minor tweaks to the original code here: - using generics for type-safety - supporting variadic arguments for convenience / ergonomics - removing some unneeded gitaly packages from the test setup - Using the above `chunk` utility in the Symbols service to divide up the list of returned symbols into ~ 1MB batches and sending it across the result stream: sourcegraph/sourcegraph@77fc77e --- Note that this PR doesn't "fix" the real issues with the underlying application itself. Notably: - The server is still calculating the entire result set in one shot (requiring it to hold all the symbols in a contiguous chunk of memory), as opposed to sending out incremental progress. - Is it necessary for the server to return thousands upon thousands of symbols, or should it support some sort of "limit" parameter? - The custom symbols client API still returns all these symbols in one giant slice (by joining all the received messages), so it still allocates a lot of memory - same as before as opposed to some sort of API where the result can be consumed incrementally. However, we still aren't any worse off than we were before (the REST implementation still has the same memory allocation problems and sends the results in one giant JSON message). This PR does "work around" the gRPC-specific message size complaints while providing a building block to improve this in the future. cc @sourcegraph/code-intel ^ ## Test plan 1. CI 2. Manual testing: - Using the [repository and file mentioned in this log message](https://console.cloud.google.com/logs/query;cursorTimestamp=2023-07-24T15:43:57.509461420Z;duration=P7D;query=resource.type%3D%22k8s_container%22%0Aresource.labels.project_id%3D%22sourcegraph-dev%22%0Aresource.labels.location%3D%22us-central1-f%22%0Aresource.labels.cluster_name%3D%22cloud%22%0Aresource.labels.namespace_name%3D%22prod%22%0AjsonPayload.InstrumentationScope%3D~%22gRPC.internal.error.reporter%22%0A-jsonPayload.message%3D%22grpc:%20error%20while%20marshaling:%20string%20field%20contains%20invalid%20UTF-8%22%0A-jsonPayload.Body%3D%22grpc:%20error%20while%20marshaling:%20string%20field%20contains%20invalid%20UTF-8%22%0Atimestamp%3D%222023-07-24T15:43:57.509461420Z%22%0AinsertId%3D%22f964g54awjrevxzi%22?project=sourcegraph-dev), run a local sourcegraph instance with the following diff applied (to simulate the absence of the chunker by setting the message size to 1 TB) ```diff diff --git a/internal/grpc/chunk/chunker.go b/internal/grpc/chunk/chunker.go index 947b721368..c954854fc6 100644 --- a/internal/grpc/chunk/chunker.go +++ b/internal/grpc/chunk/chunker.go @@ -42,7 +42,7 @@ type Chunker[T Message] struct { } // maxMessageSize is the maximum size per protobuf message -const maxMessageSize = 1 * 1024 * 1024 +const maxMessageSize = 1 * 1024 * 1024 * 1024 * 1024 ``` - Nagivate to the above-mentioned repository and file, and hover over any token - see the expected log message: ``` [ frontend] ERROR symbolsConnectionCache.gRPC.internal.error.reporter.streamingMethod.postMessageReceive internalerrs/logging.go:239 grpc: received message larger than max (708588568 vs. 94371840) {"grpcService": "symbols.v1.SymbolsService", "grpcMethod": "LocalCodeIntel", "grpcCode": "ResourceExhausted", "initialRequestJSON": "[omitted]"} ``` - Revert the above diff (which should restore the 1MB chunking behavior), and hard-refresh the blob page and hover over the same token. Eventually (after 30s) on my machine, the request will complete and a hover will appear - no error occurs. <br> Backport 2a6c569 from #55242
camdencheek
pushed a commit
that referenced
this issue
Jul 28, 2023
This sanitizes sources of the `Content` field of `ChunkMatch` to ensure that it is always valid UTF-8. The underlying issue is that searcher does not exclude utf8 files when searching, so we can't guarantee that the contents are valid utf8.
camdencheek
pushed a commit
that referenced
this issue
Jul 31, 2023
…ain (#55441) In https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph/-/commit/591d702d290c5a998bc08c4e4e66c8914407d542, we moved this init script from enterprisecmd to frontend, which disabled the enterprise migrations to be considered in other services - namely worker which is running the migrations in the background for active instances. That means that we haven't been running a few migrations while instances are running in 5.1, but that shouldn't be a big problem luckily because - none are deprecated in 5.1 - none were introduced in 5.1 - only a few from 5.0 would potentially need to run - but they likely already completed while the instance was on 5.0, unless there was a lot of stuff to be migrated - when an instance first booted on 5.1, these would have never run, but also no old data would exist
coury-clark
pushed a commit
that referenced
this issue
Aug 1, 2023
Original Slack thread [here](https://sourcegraph.slack.com/archives/C05EMJM2SLR/p1687969384430849) ## Before this PR We use a `min_access_level=20` check on GitLab project queries, because `access_level` 10 is GUEST, and users with GUEST access cannot see repo contents, so we don't want to give access to those repos on Sourcegraph HOWEVER, for Internal repos on GitLab, users can see the contents of the repo depending on how the repo is configured, regardless of their access level. So this puts us in a conundrum. If we have `min_access_level=20`, users can't see repos they should be able to see, but if we remove it, users can see repos they shouldn't be able to see ## After this PR Adds a feature flag named "gitLabProjectVisibilityExperimental". When set to true: @indradhanush and I diffed some responses from the GitLab API, depending on whether or not you are part of the GitLab group or not, whether the project is visible or not, etc. etc. and there is a common field: `default_branch` is only visible in the project API response if the user is able to see the contents of the project. If they cannot see the contents of the project, they cannot see the default branch of the project. So by removing `min_access_level=20` and checking for the presence of `default_branch` on the returned project list, we can determine whether or not the user should be able to see the repo contents or not ## Test plan VCR tests should be updated <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles --> <br> Backport 8cb61f8 from #54426 Co-authored-by: Petri-Johan Last <petri.last@sourcegraph.com>
camdencheek
pushed a commit
that referenced
this issue
Aug 2, 2023
…sages sent by servers/clients (#55495) Follow up to https://github.com/sourcegraph/sourcegraph/pull/55209 and https://github.com/sourcegraph/sourcegraph/pull/55242. This PR adds interceptors that records Prometheus metrics that observe: - the individual size of each **sent** protobuf message by a server or client - the total amount data sent over the course a single RPC by a server (responses) or client (requests) This allows us to track the total amount of a data returned by any of our RPCs. In some cases, this can reveal opportunities for future performance / stability improvements (Example: symbols' [LocalCodeIntel method returning ~gigabyte sized responses that has to be held all at once in memory](https://github.com/sourcegraph/sourcegraph/pull/55242)). This PR also provides new grafana dashboards that track this metric for every gRPC service. See below for a screenshot of what this looks like when I run the symbols service locally. Co-authored-by: Geoffrey Gilmore <geoffrey@sourcegraph.com>
coury-clark
pushed a commit
that referenced
this issue
Aug 7, 2023
In https://github.com/sourcegraph/sourcegraph/pull/55498, I removed the extra namespace from all the gRPC service metrics. However, I missed that the method variable (which we use to be able to filter metrics for an individual method) definition was still referencing the old namespace. This had the inadvertent effect of breaking most of the gRPC dashboards on sourcegraph.com - since the metrics they were looking for didn't exist. This PR fixes this by replacing the namespace in the definition with a grpc_service label filter. ## Test plan Local testing <br> Backport 3caeed8 from #55531 Co-authored-by: Geoffrey Gilmore <geoffrey@sourcegraph.com>
coury-clark
pushed a commit
that referenced
this issue
Aug 8, 2023
Fixes the root cause of #55414, which broke upgrades between `v5.0[3,6]` and `v5.1.5`. We will need to provide instructions for users that did hit v5.1.5, as they will have already experienced the effect of this bug. They can simply run the missing [migrations](https://github.com/sourcegraph/sourcegraph/issues/55414#issuecomment-1669794219) we can supply as a support packet. This should fix the issue from occurring in `v5.1.6` and later. The issue came from v5.1.5 incorrectly backfilling a set of migrations. When we initialize the database, we insert "backfilled" migration records for migrations that existed in the past but are no longer in-tree (after being squashed away). The logic for this assumed that version branches wouldn't redefine migration ancestry (we still shouldn't, but we can fix the effect of there). Since we've started backporting Cody features, we've introduced [at least one](#50869) occurrence where the ancestry of a migration definition changes between the `5.0` and `main` branches. In these cases, a migration was backported without all of its ancestors. This creates a situation where moving from `5.0` to `5.1` branches a migration suddenly gets an (unapplied) parent added to it. This breaks the assumption that any migration successfully applied has also had its ancestors applied. But sometimes Fry has to go back in time and become his own grandfather. 🤷 This PR fixes the issue by instead of backfilling _all ancestors of all applied migrations-, we only backfill the (get read for this, it's a mouth-full): _all ancestors of the root of a version of which all leaf migrations have been applied_. :clueless: This is a less eager way to backfill, as we will only backfill definitions prior to a *completely applied* set of migrations for some particular version. ## Test plan Updated unit tests. Will backport these changes to a local 5.1 branch and test the v5.0.6->v5.1.5(+ this fix) upgrade to see if we still have issues. <br> Backport a59fd1e from #55650 Co-authored-by: Eric Fritz <eric@sourcegraph.com>
sanderginn
pushed a commit
that referenced
this issue
Aug 10, 2023
The aggregatedCodyUsageEventsQuery is timing out for dotcom. These changes are cte optimization steps. ## Test plan Extract query from most expensive ping-supporting query, [aggregatedCodyUsageEventsQuery](https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/database/event_logs.go?L1594), analyze original query: ``` GroupAggregate (cost=143042.53..143382.23 rows=270 width=158) (actual time=492.653..608.924 rows=87 loops=1) Group Key: event_logs.name, (date_trunc('month'::text, '2023-08-01 17:00:00-07'::timestamp with time zone)), ((date_trunc('week'::text, ('2023-08-01 17:00:00-07'::timestamp with time zone + '1 day'::interval)) - '1 day'::interval)), (date_trunc('day'::text, '2023-08-01 17:00:00-07'::timestamp with time zone)) CTE code_generation_keys -> Function Scan on unnest key (cost=0.00..0.09 rows=9 width=32) (actual time=0.005..0.007 rows=9 loops=1) CTE explanation_keys -> Function Scan on unnest key_1 (cost=0.00..0.05 rows=5 width=32) (actual time=0.012..0.013 rows=5 loops=1) -> Sort (cost=143042.38..143046.21 rows=1530 width=62) (actual time=492.526..496.803 rows=33511 loops=1) Sort Key: event_logs.name Sort Method: external merge Disk: 3056kB -> Gather (cost=1000.16..142961.45 rows=1530 width=62) (actual time=12.381..418.370 rows=33511 loops=1) Workers Planned: 2 Workers Launched: 2 -> Parallel Seq Scan on event_logs (cost=0.17..141808.45 rows=638 width=62) (actual time=10.540..417.177 rows=11170 loops=3) Filter: ((name !~~ '%completion:suggested%'::text) AND (name !~~ '%completion:started%'::text) AND (name !~~ '%CTA%'::text) AND (name !~~ '%Cta%'::text) AND (NOT (hashed SubPlan 9)) AND (lower(name) ~~ '%cody%'::text) AND ("timestamp" >= (date_trunc('month'::text, '2023-08-01 17:00:00-07'::timestamp with time zone) - '1 mon'::interval))) Rows Removed by Filter: 382174 SubPlan 9 -> Function Scan on unnest (cost=0.00..0.13 rows=13 width=32) (actual time=0.007..0.008 rows=13 loops=3) SubPlan 8 -> CTE Scan on explanation_keys explanation_keys_3 (cost=0.00..0.10 rows=5 width=32) (actual time=0.000..0.001 rows=5 loops=1) SubPlan 7 -> CTE Scan on explanation_keys explanation_keys_2 (cost=0.00..0.10 rows=5 width=32) (actual time=0.014..0.016 rows=5 loops=1) SubPlan 6 -> CTE Scan on explanation_keys explanation_keys_1 (cost=0.00..0.10 rows=5 width=32) (actual time=0.000..0.001 rows=5 loops=1) SubPlan 5 -> CTE Scan on code_generation_keys code_generation_keys_1 (cost=0.00..0.18 rows=9 width=32) (actual time=0.000..0.002 rows=9 loops=1) SubPlan 4 -> CTE Scan on explanation_keys (cost=0.00..0.10 rows=5 width=32) (actual time=0.000..0.001 rows=5 loops=1) SubPlan 3 -> CTE Scan on code_generation_keys (cost=0.00..0.18 rows=9 width=32) (actual time=0.007..0.010 rows=9 loops=1) Planning Time: 1.556 ms Execution Time: 610.143 ms ``` Add indexes and analyze again in comments. <br> Backport 41e5cf3 from #55538 --------- Co-authored-by: Nathan Downs <85511556+nathan-downs@users.noreply.github.com>
DaedalusG
pushed a commit
that referenced
this issue
Jun 6, 2024
…sync (#63131) In the current implementation, we check if there's a user in the database already, and if so we add a random suffix to increase the chances of the insert/update to succeed. However, we didn't check if the user that already exists is _the same user_. So for a first sync, usernames would look nice and tidy, but then on a second sync from the SCIM provider, every user would get a suffix. This PR fixes that by adding a check for the user ID. Test plan: Added tests, verified SCIM sync doesn't modify usernames anymore locally. <br> Backport 614375e from #63122 Co-authored-by: Erik Seliger <erikseliger@me.com>
craigfurman
pushed a commit
that referenced
this issue
Jun 10, 2024
…63178) the executor image and docker mirror image should now follow the following naming convention: Image family: `sourcegraph-executors-[nightly|internal|'']-<MAJOR>-<MINOR>` Image name: `sourcegraph-executor-[nightly|internal|'']-<MAJOR>-<MINOR>-<BUILD_NUMBER>` example: Image family: `sourcegraph-executors-5-4` Image name: `sourcegraph-executor-5-4-277666` ## What happens during releases and _not_ releases? #### Nightly **`nightly` suffix** Image family: `sourcegraph-executors-nightly-<MAJOR>-<MINOR>` Image name: `sourcegraph-executor-nightly-<MAJOR>-<MINOR>-<BUILD_NUMBER>` #### Internal **`internal` suffix** Image family: `sourcegraph-executors-internal-<MAJOR>-<MINOR>` Image name: `sourcegraph-executor-internal-<MAJOR>-<MINOR>-<BUILD_NUMBER>` #### Public / Promote to public ** No suffix ** Image family: `sourcegraph-executors-<MAJOR>-<MINOR>` Image name: `sourcegraph-executor-<MAJOR>-<MINOR>-<BUILD_NUMBER>` > [!IMPORTANT] > Should we keep the imagine name stable at `sourcegraph-executor-<MAJOR>-<MINOR>-<BUILD_NUMBER>` > and only change the family name? > > **Why?** > > The Image family dictates the collection of images and that changes each major minor and or release phase so there is really no use in changing the image name too, except at a glance you can see from the name what image family it belongs to? ## Test plan ## Changelog <br> Backport 8bb0ab5 from #63157 Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
jdpleiness
pushed a commit
that referenced
this issue
Jul 11, 2024
… generated config (#63793) The OTEL upgrade https://github.com/sourcegraph/sourcegraph/pull/63171 bumps the `prometheus/common` package too far via transitive deps, causing us to generate configuration for alertmanager that altertmanager doesn't accept, at least until the alertmanager project cuts a new release with a newer version of `promethues/common`. For now we forcibly downgrade with a replace. Everything still builds, so we should be good to go. ## Test plan `sg start` and `sg run prometheus`. On `main`, editing `observability.alerts` will cause Alertmanager to refuse to accept the generated configuration. With this patch, all is well it seems - config changes go through as expected. This is a similar test plan for https://github.com/sourcegraph/sourcegraph/pull/63329 ## Changelog - Fix Prometheus Alertmanager configuration failing to apply `observability.alerts` from site config <br> Backport ffa873f from #63790 Co-authored-by: Robert Lin <robert@bobheadxi.dev>
jdpleiness
pushed a commit
that referenced
this issue
Jul 11, 2024
This PR fixes an important bug in #62976, where we didn't properly map the symbol line match to the return type. Instead, we accidentally treated symbol matches like file matches and returned the start of the file. ## Test plan Add new unit test for symbol match conversion. Extensive manual testing. <br> Backport 004eb0f from #63773 Co-authored-by: Julie Tibshirani <julietibs@apache.org>
jdpleiness
pushed a commit
that referenced
this issue
Jul 17, 2024
This PR upgrades the cody web experimental package to 0.2.5, in the new version we fixed - Telemetry problem with init extension-related events (we don't send install extension events anymore) - Most recent updates on LLM availability for enterprise instances ## Test plan - CI is green - Manual check on basic Cody Web functionality (highly recommended) <br> Backport e6bd85e from #63839 Co-authored-by: Vova Kulikov <vovakulikov@icloud.com>
jdpleiness
pushed a commit
that referenced
this issue
Jul 22, 2024
…63987) In order to run nightly vulnerability scans of Sourcegraph releases, we need to publish a new set of images whenever the release branch is pushed to. Previously, this was implemented in https://github.com/sourcegraph/sourcegraph/pull/63379 but with RFC 795 the release branch format changed from 5.5.1234 to 5.5.x. This PR updates the regex to catch this new format. The end result of this is that whenever Buildkite runs on a branch matching `\d.\d.x`, it will push images to the `us.gcr.io/sourcegraph-dev/gitserver` registry with the tag `$branch-insiders`. I've also tagged this PR for backport as we want it on the current patch release branch 5.5.x :) ## Test plan - Test buildkite run on branch `will-0.0.x` (with modified regex to match that branch) https://buildkite.com/sourcegraph/sourcegraph/builds/283608 ## Changelog <br> Backport b7242d2 from #63985 Co-authored-by: Will Dollman <will.dollman@sourcegraph.com>
willdollman
added a commit
that referenced
this issue
Jul 24, 2024
…eline (#64030) As part of the [Vuln Scanning Improvements](https://linear.app/sourcegraph/project/[p0]-vulnerability-scanning-improvements-75299c4312dd/issues) project, I've been working on tooling to automate the security approval step of the release process. This PR integrates these improvements into the release pipeline: * Internal releases will run a vulnerability scan * Promote-to-public releases will check for security approval If a public release does not have security approval, it will block the promotion process. The step happens at the start of the pipeline so should be a fast-fail. You can also check for release approval before running promotion by running `@secbot cve approve-release <version>` in the #secbot-commands channel. In an ideal world we (security) will have already gone through and approved ahead of release. I've tested this PR as much as I can without running an actual release! We have a 5.5.x release tomorrow so it'll be a good test. If it does cause problems that can't be easily solved, it can always be temporarily disabled. I've tagged this PR to be backported to `5.5.x`. ## Pre-merge checklist - [x] Revert commit that disables release promotion ## Test plan Manual testing of the release process: - [x] [Successful test run](https://buildkite.com/sourcegraph/sourcegraph/builds/283774#0190dfd6-fa70-4cea-9711-f5b8493c7714) that shows the security scan being triggered - [x] [Promote to public test run](https://buildkite.com/sourcegraph/sourcegraph/builds/283826) that shows the security approval approving a release - [x] [Promote to public test run](https://buildkite.com/sourcegraph/sourcegraph/builds/283817#0190e0ec-0641-4451-b7c7-171e664a3127) that shows the security approval rejecting a release with un-accepted CVEs ## Changelog <br> Backport 9dd901f from #63990 Co-authored-by: Will Dollman <will.dollman@sourcegraph.com>
arafatkatze
added a commit
that referenced
this issue
Aug 8, 2024
…s to default to true (#64371) Changing azureUseDeprecatedCompletionsAPIForOldModels to default to trueso that customers who upgrade don't have to change siteadmin config <img width="323" alt="image" src="https://github.com/user-attachments/assets/a690b54b-aa43-43c3-9f7d-202f161866bc"> ## Test plan Tested locally ## Changelog <br> Backport 3b16059 from #64347 Co-authored-by: Ara <arafat.da.khan@gmail.com>
camdencheek
pushed a commit
that referenced
this issue
Aug 8, 2024
closes: https://linear.app/sourcegraph/issue/SRCH-866/improve-cody-web-theming-for-consistency-with-rest-of-product This PR updates the Cody Web CSS based on the changes requested by @rrhyne. Few buttons implemented in Cody doesn't satisfy the design requirements and do not use the css variable so we couldn't update them easily but rather had to override the styles. ## Test plan Before: ![CleanShot 2024-08-08 at 22 53 48@2x](https://github.com/user-attachments/assets/95e10b42-b740-4663-a761-69449ec62296) After: ![CleanShot 2024-08-08 at 22 54 12@2x](https://github.com/user-attachments/assets/bffbd10a-c87b-4eca-9582-b23eebccb60e) ## Changelog - Make Cody Web styles more accessible. <br> Backport 2dd38b3 from #64370 Co-authored-by: Naman Kumar <naman@outlook.in>
fkling
added a commit
that referenced
this issue
Aug 9, 2024
…e definitions and 'find implementations' (#64390) This commit adds the necessary logic to open the explorer tab for multiple references and for implementations. I basically copied what @camdencheek did for the references tab. ## Test plan Hover over `adapt` in http://localhost:5173/github.com/sveltejs/kit@65931f276ac2102032e3032c864a472eee19b7bb/-/blob/packages/kit/src/exports/vite/index.js?L890 and click 'got to definition'. The explore panel with should open with the definitions tab selected. I wasn't able to test the find implementations logic because I don't know for which language we have this implemented. <br> Backport bcc816a from #64349 Co-authored-by: Felix Kling <felix@felix-kling.de>
fkling
added a commit
that referenced
this issue
Aug 9, 2024
…nfo (#64391) I don't know why but having `overflow-x` causes the content of the hovercard to not fully expand to the edges. It has something to do with the sizing of the `hr` element. | Before | After | |--------|--------| | ![2024-08-07_23-39_1](https://github.com/user-attachments/assets/5e02f369-bc33-49db-b69d-ec63afe5de17) | ![2024-08-07_23-39](https://github.com/user-attachments/assets/04c71fff-a232-4c8d-b0a5-3ea1d1659e29) | ## Test plan Manual testing. <br> Backport cf54671 from #64350 Co-authored-by: Felix Kling <felix@felix-kling.de>
fkling
added a commit
that referenced
this issue
Aug 9, 2024
…vigating to a different page (#64392) Fixes srch-859 Problem: Currently the sidebar can reopen in the following scenario: - Go to search home and submit a search - On the search results page click the menu icon to open the sidebar and click 'Code Search' to go back to search home. - Execute a different query - -> this brings you back to search results but with the menu sidebar open Looking at the code it's understandable that this happens: We never close the sidebar. But this problem wasn't very visible because every entry in the nav sidebar either brings you to the React app or to a page with inline nav menu. So this commit fixes the issue by closing the sidebar whenever we navigate to a different page. ## Test plan Manual testing. I added a temporary nav entry that would bring me to the sourcegraph repo page. Clicking on it from the search results page caused the sidebar to close. <br> Backport 97ef93d from #64357 Co-authored-by: Felix Kling <felix@felix-kling.de>
fkling
added a commit
that referenced
this issue
Aug 9, 2024
Fixes srch-858 Currently the menu icon rotates when hovering over it. The animation should only target the Sourcegraph logo. Additional changes: - Move icon color style prop into CSS declaration (avoids inserting an additional element) - Let the animation target the link instead of the icon. There doesn't seem to be reason why we actually need to target the `svg` element via `:global`. ## Test plan Manual testing. <br> Backport 975b14f from #64358 Co-authored-by: Felix Kling <felix@felix-kling.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
feature-request
stale
team/graph
Graph Team (previously Code Intel/Language Tools/Language Platform)
From the user:
The text was updated successfully, but these errors were encountered: