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: add prometheus server and client prometheus metrics #642

Merged
merged 7 commits into from
Sep 12, 2023

Conversation

ggilmore
Copy link
Contributor

@ggilmore ggilmore commented Sep 6, 2023

This PR does a few things (each commit is independently reviewable)

  • rename webserver proto package from "grpc.v1" to "zoekt.webserver.v1" - this is for consistency
    • Renaming / moving the package made me realize that buf lint never` worked properly on this package. I also went ahead and fixed all the lint complaints.
  • adds Prometheus gRPC server and client interceptors from https://github.com/grpc-ecosystem/go-grpc-middleware/tree/main/providers/prometheus, which implements standardized timing and error rates for the zoekt-webserver service and zoekt-sourcegraph-indexserver client

See https://github.com/sourcegraph/sourcegraph/pull/56525 for a screenshot of what the new zoekt-webserver gRPC dashboards look like in sourcegraph.

@ggilmore ggilmore force-pushed the introduce-prometheus-monitoring branch from 347ab0e to de3f7cb Compare September 6, 2023 17:09
this will be changed to point to zoekt@main once #642 is merged.
ggilmore added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this pull request Sep 6, 2023
this will be changed to point to zoekt@main once sourcegraph/zoekt#642 is merged.
@ggilmore ggilmore marked this pull request as ready for review September 12, 2023 01:28
@ggilmore ggilmore requested review from camdencheek, keegancsmith and stefanhengl and removed request for keegancsmith September 12, 2023 01:28
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

github.com/sourcegraph/zoekt/cmd/zoekt-webserver/grpc/protos/zoekt/webserver/v1

why not one of the following

  • github.com/sourcegraph/zoekt/proto
  • github.com/sourcegraph/zoekt/web/proto

Nesting under cmd is a bit strange, especially for others to import. Makes somewhat sense in our monorepo, but not really for most projects.

More context: These are generating very deeply nested package names. Is that some sort of convention we are following? In the past I have just seen something simple like a proto pkg. Personally for a small project like zoekt keeping it simple feels good to me. But if this is a convention we are wanting to follow then ignore me.

cmd/zoekt-sourcegraph-indexserver/main_test.go Outdated Show resolved Hide resolved
@ggilmore
Copy link
Contributor Author

github.com/sourcegraph/zoekt/cmd/zoekt-webserver/grpc/protos/zoekt/webserver/v1

why not one of the following

* github.com/sourcegraph/zoekt/proto

* github.com/sourcegraph/zoekt/web/proto

Nesting under cmd is a bit strange, especially for others to import. Makes somewhat sense in our monorepo, but not really for most projects.

More context: These are generating very deeply nested package names. Is that some sort of convention we are following? In the past I have just seen something simple like a proto pkg. Personally for a small project like zoekt keeping it simple feels good to me. But if this is a convention we are wanting to follow then ignore me.

Two things:

First: the package name services as a unique identifer for the service in question:

WebserverService_Search_FullMethodName = "/zoekt.webserver.v1.WebserverService/Search"

zoekt.webserver.v1.WebserverService is the full name of the service. The service name is how you target this particular service when using other gRPC tools (like our Grafana dashboards, clients, etc.). So, it was important to make sure that it is globally unique and identifiable.

Second: In GRPC, the folder structure also must mirror the package name. zoekt.webserver.v1 implies that there is a zoekt/webserver/v1 folder hierarchy. (we can't use the root of the repository git checkout as the initial folder since you can change the name to be whatever you want, so we need to explicitly create this nested folder hierarchy)

The protobuf file resolution process (that compilers, Wireshark, etc. all use) depends on this folder hierarchy being correct.

So between those two points, we need the zoekt/webserver/v1 folders somewhere in the repository. However, I did move it from cmd/zoekt-webserver to grpc/protos after your comment.

@ggilmore ggilmore merged commit 3ce1f2b into main Sep 12, 2023
7 checks passed
@ggilmore ggilmore deleted the introduce-prometheus-monitoring branch September 12, 2023 15:24
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