Skip to content
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

grpc: port internal error interceptors from sourcegraph/sourcegraph #639

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

ggilmore
Copy link
Contributor

@ggilmore ggilmore commented Aug 18, 2023

This PR ports the internal error gRPC interceptors that are used for monitoring and debugging in sourcegraph/sourcegraph to the Zoekt project.

Differences from the main implementation:

  • The prometheus metric names removed the src prefix to make it suitable for use outside of sourcegraph.

    • Ex. src_grpc_method_status -> grpc_method_status
    • (I'll need to tweak Sourcegrpah's GRPC Grafana dashboard generation logic to now allow for namespaced metrics).
  • The environment variables have the SRC prefix removed to make it suitable for use outside of sourcegraph.

    • E.X. SRC_GRPC_INTERNAL_ERROR_LOGGING_ENABLED -> GRPC_INTERNAL_ERROR_LOGGING_ENABLED
  • The environment variables use small custom mustGet[Bool/Bytes] methods since the env package from sourcegraph/sourcegraph doesn't exist in Zoekt

testing

Beyond the CI tests, I also manually patched Zoekt to force a non-utf8 repository name in the stream search implementation.

diff --git a/grpc/server/server.go b/grpc/server/server.go
index b034381..1b59f03 100644
--- a/grpc/server/server.go
+++ b/grpc/server/server.go
@@ -86,6 +86,9 @@ func gRPCChunkSender(ss v1.WebserverService_StreamSearchServer) zoekt.Sender {
 
 		sendFunc := func(filesChunk []*v1.FileMatch) error {
 			numFilesSent += len(filesChunk)
+			filesChunk = append(filesChunk, &v1.FileMatch{
+				Repository: "\xff", 
+			})
 
 			var stats *v1.Stats
 			if !statsSent { // We only send stats back on the first chunk

When I ran sourcegraph using the patched zoekt:

replace (
	github.com/sourcegraph/zoekt => ../zoekt
)

I saw the following logs (notice the nonUTF8StringFields in the zoekt-webserver output):

[       frontend] ERROR zoekt.gRPC.internal.error.reporter.streamingMethod.postMessageReceive internalerrs/logging.go:240 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "grpc.v1.WebserverService", "grpcMethod": "StreamSearch", "grpcCode": "Internal", "initialRequestJSON": "{\"query\":{\"Query\":{\"And\":{\"children\":[{\"Query\":{\"BranchesRepos\":{\"list\":[{\"branch\":\"HEAD\",\"repos\":\"OjAAAAEAAAAAAAAAEAAAAAcA\"}]}}},{\"Query\":{\"Symbol\":{\"expr\":{\"Query\":{\"Substring\":{\"pattern\":\"te\"}}}}}}]}}},\"opts\":{\"shard_max_match_count\":10000,\"total_max_match_count\":100000,\"max_wall_time\":{\"seconds\":59,\"nanos\":999356584},\"flush_wall_time\":{\"nanos\":500000000},\"max_doc_display_count\":50,\"chunk_matches\":true,\"use_document_ranks\":true,\"document_ranks_weight\":4500}}", "nonUTF8StringFields": [], "messageJSON": "{}"}
[    zoekt-web-1] ERROR ZoektWebserverGRPCServer.gRPC.internal.error.reporter.streamingMethod.postMessageSend internalerrs/logging.go:236 grpc: error while marshaling: string field contains invalid UTF-8 {"grpcService": "grpc.v1.WebserverService", "grpcMethod": "StreamSearch", "grpcCode": "Internal", "nonUTF8StringFields": ["files[50].repository"]}

@ggilmore ggilmore marked this pull request as ready for review August 18, 2023 17:41
Copy link
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not review closely since this is just copy/past of existing, reviewed code.

@ggilmore ggilmore merged commit 993cfdb into main Aug 18, 2023
7 checks passed
@ggilmore ggilmore deleted the port-internal-error branch August 18, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants