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.

Sign up for GitHub

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

Jump to bottom

feat: allow using custom debug write functions #506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: allow using custom debug write functions #506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions hcloud/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ type Client struct {
applicationName string
applicationVersion string
userAgent string
debugWriter io.Writer
debugOpts *DebugOpts
instrumentationRegistry prometheus.Registerer
handler handler

Expand Down Expand Up @@ -216,7 +216,28 @@ func WithApplication(name, version string) ClientOption {
// writer. To, for example, print debug information on stderr, set it to os.Stderr.
func WithDebugWriter(debugWriter io.Writer) ClientOption {
return func(client *Client) {
client.debugWriter = debugWriter
client.debugOpts = defaultDebugOpts(debugWriter)
}
}

// DebugOpts defines the options used by [WithDebugOpts].
type DebugOpts struct {
WriteRequest func(id string, dump []byte)
WriteResponse func(id string, dump []byte)
}
Comment on lines +223 to +227
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if this is the right abstraction or if we should instead use OpenTelemetry Logger or log/slog.Logger

Copy link
Member Author

@jooola jooola Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about using slog directly, but this gives us more flexibility. We might need to support different logging framework, or format the dump before printing it out, having those callbacks allows it.

If we use slog, we might have to use a dedicated slog Handler to be able to get each key=value of a record, and then be able to print it out. This feel overkill in comparison to the callbacks. Note that the callbacks does allow using slogs, but the other way around is less straight forward.

I think adding open telemetry is a different topic, which I prefer not to mix with our dead simple debug output.

This PR can be backported to v1, while the open telemetry one, maybe less so.

That said, if we prefer to be conservative and not add yet another API for us to maintain over the years, I totally understand it and will try to implement open telemetry right away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not pass a map[string]any so we could support more fields in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer not have an untyped map to pass more data. If we need more flexibility in the argument list, I'd rather use a struct.

What additional data do you think we will want to pass in the future ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also allow the user to hook its own handlers inside the chain, but I am afraid to give that much freedom to the users of the Client, as it will be way easier to mess with the request/response objects.


func defaultDebugOpts(writer io.Writer) *DebugOpts {
return &DebugOpts{
WriteRequest: defaultDebugWriter(writer, "Request"),
WriteResponse: defaultDebugWriter(writer, "Response"),
}
}

// WithDebugOpts configures a Client to print debug information using the given write
// functions.
func WithDebugOpts(opts DebugOpts) ClientOption {
return func(client *Client) {
client.debugOpts = &opts
}
}

Expand Down
6 changes: 3 additions & 3 deletions hcloud/client_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ func assembleHandlerChain(client *Client) handler {
// Start down the chain: sending the http request
h := newHTTPHandler(client.httpClient)

// Insert debug writer if enabled
if client.debugWriter != nil {
h = wrapDebugHandler(h, client.debugWriter)
// Insert debug opts if enabled
if client.debugOpts != nil {
h = wrapDebugHandler(h, client.debugOpts)
}

// Read rate limit headers
Expand Down
46 changes: 40 additions & 6 deletions hcloud/client_handler_debug.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,38 @@
package hcloud

import (
"bytes"
"context"
"crypto/rand"
"encoding/hex"
"fmt"
"io"
"net/http"
"net/http/httputil"
)

func wrapDebugHandler(wrapped handler, output io.Writer) handler {
return &debugHandler{wrapped, output}
type debugWriteFunc func(id string, dump []byte)

func wrapDebugHandler(
wrapped handler,
opts *DebugOpts,
) handler {
return &debugHandler{
handler: wrapped,
writeRequest: opts.WriteRequest,
writeResponse: opts.WriteResponse,
}
}

type debugHandler struct {
handler handler
output io.Writer
handler handler
writeRequest debugWriteFunc
writeResponse debugWriteFunc
}

func (h *debugHandler) Do(req *http.Request, v any) (resp *Response, err error) {
id := generateRandomID()

// Clone the request, so we can redact the auth header, read the body
// and use a new context.
cloned, err := cloneRequest(req, context.Background())
Expand All @@ -32,7 +47,7 @@ func (h *debugHandler) Do(req *http.Request, v any) (resp *Response, err error)
return nil, err
}

fmt.Fprintf(h.output, "--- Request:\n%s\n\n", dumpReq)
h.writeRequest(id, dumpReq)

resp, err = h.handler.Do(req, v)
if err != nil {
Expand All @@ -44,7 +59,26 @@ func (h *debugHandler) Do(req *http.Request, v any) (resp *Response, err error)
return nil, err
}

fmt.Fprintf(h.output, "--- Response:\n%s\n\n", dumpResp)
h.writeResponse(id, dumpResp)

return resp, err
}

func generateRandomID() string {
b := make([]byte, 4)
_, err := rand.Read(b)
if err != nil {
panic(fmt.Errorf("failed to generate random string: %w", err))
}
return hex.EncodeToString(b)
}

func defaultDebugWriter(output io.Writer, title string) func(id string, dump []byte) {
return func(_ string, dump []byte) {
fmt.Fprintf(output,
"--- %s:\n%s\n\n",
title,
bytes.Trim(dump, "\n"),
)
}
}
6 changes: 1 addition & 5 deletions hcloud/client_handler_debug_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ Authorization: REDACTED
Accept-Encoding: gzip



`,
},
{
Expand All @@ -47,13 +46,11 @@ Authorization: REDACTED
Accept-Encoding: gzip



--- Response:
HTTP/1.1 503 Service Unavailable
Connection: close



`,
},
{
Expand All @@ -69,7 +66,6 @@ Authorization: REDACTED
Accept-Encoding: gzip



--- Response:
HTTP/1.1 200 OK
Connection: close
Expand All @@ -85,7 +81,7 @@ Content-Type: application/json
buf := bytes.NewBuffer(nil)

m := &mockHandler{testCase.wrapped}
h := wrapDebugHandler(m, buf)
h := wrapDebugHandler(m, defaultDebugOpts(buf))

client := NewClient(WithToken("dummy"))
client.userAgent = "hcloud-go/testing"
Expand Down
2 changes: 1 addition & 1 deletion hcloud/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ func TestClientDoPost(t *testing.T) {

debugLog := new(bytes.Buffer)

env.Client.debugWriter = debugLog
env.Client.debugOpts = defaultDebugOpts(debugLog)
callCount := 0
env.Mux.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) {
auth := r.Header.Get("Authorization")
Expand Down