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

feat: add grpc tag to indicate request had been throttled #1531

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

adriantam
Copy link
Member

@adriantam adriantam commented Apr 12, 2024

Description

We need to convey request has been throttled so that caller can distinguish whether timeout is due to throttling.
Note that we cannot return timeout error in the distpatch throttler itself because the caller will not wait for error response in case of context timeout.

The caller (for example), the user of the OpenFGA library, may choose to use this grpcTag ("Throttled") to modify the error message returned due to request timeout. They may also update the returned header to indicate the request has been throttled.

References

Continuation of #1440

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

@adriantam adriantam requested a review from a team as a code owner April 12, 2024 18:57
@adriantam adriantam marked this pull request as draft April 12, 2024 19:01
Copy link

codecov bot commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.06%. Comparing base (d12d58b) to head (28be398).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1531      +/-   ##
==========================================
+ Coverage   86.06%   86.06%   +0.01%     
==========================================
  Files          85       85              
  Lines        8038     8039       +1     
==========================================
+ Hits         6917     6918       +1     
  Misses        790      790              
  Partials      331      331              

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

@adriantam adriantam marked this pull request as ready for review April 12, 2024 19:06
@miparnisari
Copy link
Member

We need to convey request has been throttled so that caller can distinguish whether timeout is due to throttling.

Can you ellaborate on this in the PR description? Who is the caller that will take action based on this tag? And what will that action be?

FWIW, today, any context tags are logged automatically. Also once you apply a tag to a context, any parent or children contexts also get that tag.

@adriantam
Copy link
Member Author

We need to convey request has been throttled so that caller can distinguish whether timeout is due to throttling.

Can you ellaborate on this in the PR description? Who is the caller that will take action based on this tag? And what will that action be?

FWIW, today, any context tags are logged automatically. Also once you apply a tag to a context, any parent or children contexts also get that tag.

Done.

@miparnisari
Copy link
Member

the user of the OpenFGA library, may choose to use this grpcTag ("Throttled") to modify the error message returned due to request timeout

This won't work if a person uses OpenFGA as a library and doesn't wrap it within a server. The grpc tags middleware must be running. I.e. this doesn't print anything:

    ctx := context.Background()

	grpc_ctxtags.Extract(ctx).Set("key-a", true)

	tags := grpc_ctxtags.Extract(ctx)
	for k, v := range tags.Values() {
		fmt.Println("Tag: %s, Value: %s", k, v)
	}

@adriantam adriantam merged commit 6829520 into main Apr 16, 2024
15 checks passed
@adriantam adriantam deleted the feat/grpc_tag_throttled branch April 16, 2024 13:23
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

2 participants