- Notifications
You must be signed in to change notification settings - Fork1.2k
Add ctx propagation to raw api request#570
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR adds context propagation to the low-levelnewRequest helper, ensuring that trace and cancellation contexts flow through HTTP calls in the raw API client.
- Extended
newRequestto accept acontext.Contextand attach it viaWithContext - Updated
GetRawContentto pass the incomingctxintonewRequest - Adjusted error handling in
newRequestto return early on failure before setting context
Comments suppressed due to low confidence (3)
pkg/raw/raw.go:65
- Add or update unit tests to verify that the provided context is actually attached to the outgoing
http.Requestand that cancellations/timeouts are honored.
func (c *Client) GetRawContent(ctx context.Context, owner, repo, path string, opts *RawContentOpts) (*http.Response, error) {pkg/raw/raw.go:28
- Make sure to import the "context" package at the top of this file, otherwise
context.Contextwill be undefined.
func (c *Client) newRequest(ctx context.Context, method string, urlStr string, body interface{}, opts ...gogithub.RequestOption) (*http.Request, error) {pkg/raw/raw.go:67
- Review other methods in this file (or in downstream code) that call
newRequestto ensure they’ve been updated to passctxas the first argument, preventing build failures and ensuring consistent context propagation.
req, err := c.newRequest(ctx, "GET", url, nil)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just to validate, does this work if you try it locally? I assume so, but an alternative is to take the current request context and then extend it and add it back to the request.
I did test this locally and confirmed that ctx propagation is working as expected, trace info and parent-child spans are all showing up correctly. I'm a bit confused about the suggested alternative. Since we are creating a new outbound request here (not working with an incoming HTTP request), I don't think there's an existing req ctx to extend? Let me know if I'm missing something. |
2711384 intomainUh oh!
There was an error while loading.Please reload this page.
Update
newRequestto accept acontext.Contextparam and attach it to the generatedhttp.RequestusingwithContext.This change ensures that trace contexts are correctly propagated, supporting distributed tracing in the remote MCP server.