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

coordinator: uniform gRPC metric prefix #583

Merged
merged 3 commits into from
Jun 19, 2024
Merged

Conversation

burgerdev
Copy link
Contributor

@burgerdev burgerdev commented Jun 14, 2024

During a refactoring, we discovered that the gRPC metrics include the service in both the metric name and a label:

contrast_userapi_grpc_server_handling_seconds_bucket{grpc_method="GetManifests",grpc_service="userapi.UserAPI",grpc_type="unary",le="0.01"} 1

Since it's both more consistent to report all gRPC methods under the same metric, and easier to maintain the gRPC metrics when there is only one instance, we decided to move all of them under a common prefix, contrast_.

Future list of metrics contrast_attestation_failures 8 contrast_coordinator_manifest_generation 1 contrast_grpc_server_handled_total contrast_grpc_server_handling_seconds_bucket contrast_grpc_server_handling_seconds_count contrast_grpc_server_handling_seconds_sum contrast_grpc_server_msg_received_total contrast_grpc_server_msg_sent_total contrast_grpc_server_started_total

Below is the original PR description with the motivation for refactoring userapi etc.


This PR moves the gRPC handlers for userapi and recoveryapi into the authority module. In the main module remain gRPC server setup (credentials, metrics, listener).

I'm mostly driven by the increasing complexity of changing code in both authority and userapi. We had to change the Authority API a couple of times, and adjusting the userapi test accordingly is cumbersome. Given that the Authority API is closely coupled to the gRPC API, it is probably for the best to merge the two and remove the current Authority API.

To keep things simple, I'm refactoring in several steps. This is the first step, which does the bare minimum to keep the boat afloat after the move. There are still some rough edges (I left a couple of TODOs), but it already allows to resolve this and that TODO, which are blocking RFC004 - the former here and the latter in a PoC commit [here].

Next, I need to consolidate the former Authority API with the userapi, which will mostly involve moving code around, and adapt the tests. Fortunately, these changes can now be interleaved with the recovery effort.

Review Recommendation

I split the PR into

  • a pure move of userapi*.go and recoveryapi.go
  • subsequent changes to adapt to their new habitat

It is probably easier to restrict the Changes from in the review tab to the second commit, so that you only see the actual changes. Otherwise, git does not consider the files related and the diffs are the entire files.

TODO:

  • rename PR for changelog (user facing is the change in metric names)
  • rename (and keep) first commit
  • squash fixups

@burgerdev burgerdev added the no changelog PRs not listed in the release notes label Jun 14, 2024
@burgerdev burgerdev force-pushed the burgerdev/move-userapi branch 4 times, most recently from a1c91b3 to 484b0eb Compare June 14, 2024 21:21
@burgerdev burgerdev marked this pull request as ready for review June 15, 2024 11:56
@burgerdev burgerdev requested a review from katexochen as a code owner June 15, 2024 11:56
coordinator/main.go Outdated Show resolved Hide resolved
Copy link
Member

@katexochen katexochen left a comment

Choose a reason for hiding this comment

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

Maybe we should keep the first commit to retain history.

Copy link

github-actions bot commented Jun 18, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-06-19 06:23 UTC

@burgerdev burgerdev added changelog PRs that should be part of the release notes and removed no changelog PRs not listed in the release notes labels Jun 18, 2024
@burgerdev burgerdev force-pushed the burgerdev/move-userapi branch 3 times, most recently from 257d564 to a7335ce Compare June 18, 2024 15:38
@burgerdev burgerdev changed the title coordinator: move userapi into authority coordinator: uniform gRPC metric prefix after refactoring Jun 19, 2024
@burgerdev burgerdev changed the title coordinator: uniform gRPC metric prefix after refactoring coordinator: uniform gRPC metric prefix Jun 19, 2024
This simply moves the files without modification, in order to keep the
history intact.

*This commit is not expected to compile!*
Makes the necessary adjustments to userapi and recoveryapi after moving
them from main to the authority package.
@burgerdev burgerdev force-pushed the burgerdev/move-userapi branch from a7335ce to cf571cd Compare June 19, 2024 06:14
@burgerdev burgerdev merged commit c431922 into main Jun 19, 2024
9 checks passed
@burgerdev burgerdev deleted the burgerdev/move-userapi branch June 19, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog PRs that should be part of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants