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

Use Writer in Context.Status #1606

Merged
merged 2 commits into from
Oct 17, 2019
Merged

Conversation

willnewrelic
Copy link
Contributor

This PR is an update and fix to #625

If the Context.Writer has been replaced (with something that wraps it), the status code should be sent through the new writer.

@codecov
Copy link

codecov bot commented Oct 24, 2018

Codecov Report

Merging #1606 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1606   +/-   ##
=======================================
  Coverage   98.92%   98.92%           
=======================================
  Files          40       40           
  Lines        2229     2229           
=======================================
  Hits         2205     2205           
  Misses         12       12           
  Partials       12       12
Impacted Files Coverage Δ
context.go 99.1% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a1cf65...02d54cc. Read the comment docs.

@chrispine
Copy link

Any updates on getting this super-tiny PR merged? Thanks!

@thinkerou thinkerou added this to the 1.5 milestone Mar 1, 2019
@dmarkham
Copy link
Contributor

dmarkham commented Mar 30, 2019

@thinkerou I do not see any concerns posted here or in #625 about this PR, Do we have a reason it's not part of 1.4 milestone and just get it done?

@yharish991
Copy link

is there any update on this pull request?

@willnewrelic
Copy link
Contributor Author

I have rebased this branch on the latest master. It would love your review!

@dmarkham
Copy link
Contributor

@willnewrelic Realistically until the next release can get approved. Getting things merged/reviewed into master is problematic.

@dangkaka
Copy link
Contributor

can we merge this PR?

@yharish991
Copy link

@appleboy can you look at this PR when you get a chance?

@dmarkham
Copy link
Contributor

dmarkham commented May 14, 2019

So after digging in a little deeper the c.Status() looks like it is one of many places that interact with c.writermem directly. I'm afraid making this one change alone will just have things fail in even more strange ways. @willnewrelic you have any samples your testing with you can share?

@willnewrelic
Copy link
Contributor Author

Hey @dmarkham

I am unsure about the potential for new issues, but I believe the risk is low: Anything that replaces the Context.Writer definitely wants to know what the response code is! The current situation makes instrumentation real tough, and we get issues about it frequently (eg newrelic/go-agent#90).

@paulogustavosouza
Copy link

Hi guys, it's possible to merge it ?

@dmarkham
Copy link
Contributor

dmarkham commented Jun 6, 2019

Hey @dmarkham

I am unsure about the potential for new issues, but I believe the risk is low: Anything that replaces the Context.Writer definitely wants to know what the response code is! The current situation makes instrumentation real tough, and we get issues about it frequently (eg newrelic/go-agent#90).

Oh it's an issue I agree!
Just fixing 1 of 14+ calls to c.writermem spread out in context.go and gin.go seems like we are missing a few of the next issues people will hit. if redirectFixedPath, redirectTrailingSlash, serveError, handleHTTPRequest, ServeHTTP, Copy, and reset Methods are all going to directly use c.writermem I feel like there will be more issues to debug right after this one Status method is fixed by this patch.

What I'm saying is I think this patch is missing a few places and falls short for someone wanting to replace the c.Writer. Fixing just Status is leaving lots of holes if I'm understanding things correctly.
@appleboy @thinkerou have you guys had a chance to think on this?

@thinkerou thinkerou mentioned this pull request Jun 10, 2019
@namco1992
Copy link

Hi just checking if this issue gets any update? thanks!

@chrispine
Copy link

We'd love to see this land soon. It's been open for quite a while now. Thanks!

@purple4reina
Copy link

What I'm saying is I think this patch is missing a few places and falls short for someone wanting to replace the c.Writer. Fixing just Status is leaving lots of holes if I'm understanding things correctly.

@dmarkham I totally see what you're saying here. If we're going to fix this, we should fix it completely and not just fix one of the many problems that could arise if someone were to replace c.Writer.

What I'm wondering is if we can merge this one part of the fix now, then fix the rest of the issues in a later pull request. This will allow those users who are today replacing c.Writer in order to get instrumentation to start seeing their response codes.

@willnewrelic
Copy link
Contributor Author

Looks like two people have approved this! Thank you! Perhaps it can be merged ? 😄

@betabandido
Copy link

betabandido commented Oct 3, 2019

@dmarkham @thinkerou @appleboy Is there any reason to use c.writermem, or can all of its usages be replaced with c.Writer? If there is not, I would love to see this fixed. This issue is also preventing me from using Gin with New Relic to keep track of successful/failed transactions.

@appleboy
Copy link
Member

@thinkerou waiting for your review.

@appleboy appleboy merged commit 0ce4661 into gin-gonic:master Oct 17, 2019
@willnewrelic willnewrelic deleted the context-status branch October 17, 2019 03:27
@betabandido
Copy link

It's great to see this merged :)

@thinkerou @appleboy Are you planning to create a new release anytime soon?

newrelic-eheinlein pushed a commit to newrelic/go-agent that referenced this pull request Oct 24, 2019
newrelic-eheinlein pushed a commit to newrelic/go-agent that referenced this pull request Oct 24, 2019
ThomasObenaus pushed a commit to ThomasObenaus/gin that referenced this pull request Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.