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

Upgrade go-grpc-middleware to v2.1.0 #1599

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ilaleksin
Copy link
Contributor

@ilaleksin ilaleksin commented May 5, 2024

This PR addresses #893.
It removes dependencies to go-grpc-middleware v1.4.0 existed in logging and recovery middlewares.

Description

  1. Cloned grpc_ctxtags middleware to the project repository because it was deleted from go-grpc-middleware v2.0.0. It was required either get rid of grpc tags middleware or copy it to the openfga project to delete dependency on go-grpc-middleware without v1.4.0.

Firstly, I tried to migrate the logging middleware to logging.InjectField API introduced in v2.0.0.
I couldn't complete this migration because I didn't find a way to pass Authorization Model ID from a handler to the logging middleware. Before, it was achieved by setting a key-value to the tag map and then reading all key-value pairs on the logging layer.

New API doesn't use the embedded map (as grpc_ctxtags does). Instead, it returns a context with the injected field. I didn't find a way to deliver this new context from the handler layer to PostCall function of the logging middleware.

Therefore, I decided to clone part of grpc_ctxtags middleware to the project and keep logging data propagation the same.

  1. Upgraded grpc recovery middleware to v2. There were no breaking changes.

  2. Metrics and traces were not affected by the upgrade.

Screenshot 2024-05-05 at 23 03 07 Screenshot 2024-05-05 at 23 02 29

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@ilaleksin ilaleksin requested a review from a team as a code owner May 5, 2024 21:35
Copy link

codecov bot commented May 5, 2024

Codecov Report

Attention: Patch coverage is 92.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.42%. Comparing base (e0314fb) to head (bc9b805).

Files Patch % Lines
pkg/middleware/ctxtags/context.go 88.89% 2 Missing ⚠️
pkg/middleware/ctxtags/ctxtags.go 90.48% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1599      +/-   ##
==========================================
+ Coverage   86.41%   86.42%   +0.01%     
==========================================
  Files          86       89       +3     
  Lines        8224     8273      +49     
==========================================
+ Hits         7106     7149      +43     
- Misses        788      792       +4     
- Partials      330      332       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ilaleksin ilaleksin changed the title Draft: Upgrade go-grpc-middleware to v2.1.0 Upgrade go-grpc-middleware to v2.1.0 May 6, 2024
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.

None yet

1 participant