Skip to content

Conversation

@onelapahead
Copy link
Contributor

@onelapahead onelapahead commented Dec 30, 2021

Signed-off-by: hfuss <haydenfuss@gmail.com>
Signed-off-by: hfuss <haydenfuss@gmail.com>
Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

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

Thanks @hfuss! Looks good to me after tests and coverage passes.

Signed-off-by: hfuss <haydenfuss@gmail.com>
@onelapahead
Copy link
Contributor Author

Thanks @nguyer! @eberger727 and I will ensure E2E is fixed w/ metrics enabled and then I can cleanup the coverage / unit tests.

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Would like to understand why can't just put our own middleware in, that wraps the prometheus middleware, rather than the code copy

Signed-off-by: hfuss <haydenfuss@gmail.com>
Signed-off-by: hfuss <haydenfuss@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #371 (49b6e8d) into main (a3b5bd3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #371   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          239       239           
  Lines        12886     12886           
=========================================
  Hits         12886     12886           
Impacted Files Coverage Δ
internal/apiserver/server.go 100.00% <100.00%> (ø)

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 a3b5bd3...49b6e8d. Read the comment docs.

type statusResponseWriter struct {
// websocketResponseWriter
type websocketResponseWriter struct {
http.ResponseWriter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterbroadhurst this didn't work as expected because the inherited ResponseWriter for websocketResponseWriter ended up being a statusResponseWriter which does not implement Hijacker.

I tried climbing the "inheritance stack" with something like the following since statusResponseWriter is private:

type wrapperResponseWriter struct {
   http.ResponseWriter
}
// ...
func (w *websocketResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
	h, ok := w.ResponseWriter.(wrapperResponseWriter).ResponseWriter.(http.Hijacker)
        // ...
}

but that didn't work either so decided it was easier to fork the library with the fix and have FireFly use that instead.

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Sorry this turned into more work, but this seems like a great solution 👍

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.

Websocket: response does not implement http.Hijacker - when metrics enabled

4 participants