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(schemaregistry): allow custom http client #1030

Conversation

liveFreeOrCode
Copy link

This patch introduces the ability to set a custom base http client that will be used for the rest calls to the Schema Registry.

Common use cases include using traced http clients from telemetry providers. (This may be a viable solution for #712)

// example
package main

import (
	"net/http"

	"github.com/confluentinc/confluent-kafka-go/schemaregistry"
	httptrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/net/http"
)

func main() {
	conf := schemaregistry.NewConfig()
	conf.BaseClient = httptrace.WrapClient(new(http.Client))
	client, err := schemaregistry.NewClient(conf)
}

This patch introduces the ability to set a custom base http
client. Common use cases include using traced http clients
from telemetry providers.
@liveFreeOrCode liveFreeOrCode marked this pull request as ready for review July 20, 2023 03:09
@cla-assistant
Copy link

cla-assistant bot commented Aug 4, 2023

CLA assistant check
All committers have signed the CLA.

schemaregistry/config.go Outdated Show resolved Hide resolved
}

client.Transport = transport
client.Timeout = time.Duration(timeout) * time.Millisecond

Choose a reason for hiding this comment

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

I wonder if the "old" config should be deprecated. Also, it might make sense to check if the client has any existing timeout and use that instead if so.

Copy link
Author

Choose a reason for hiding this comment

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

I think this makes sense, but I'm hesitant to do it as part of this PR. That is more of a design change rather than a feature to enable us the ability to start using our own HTTP client. Right now the schemaregistry.Config struct is mostly an opinionated abstraction over how an http.Client is configured, and I think I want to retain that intent through this work.

If later, it's desired to allow users to more explicitly set an http.Client and all it's settings, then that work should encompass everything from the TLS, timeout and any other settings currently exposed through schemaregistry.Config.

@sagikazarmark
Copy link

This is a requirement for proper observability, but context propagation would also be necessary (ie. add a context.Context first parameter to each method or introduce new SomethingContext variants to keep backwards compatibility)

@liveFreeOrCode
Copy link
Author

This is a requirement for proper observability, but context propagation would also be necessary (ie. add a context.Context first parameter to each method or introduce new SomethingContext variants to keep backwards compatibility)

Good point. I'm leaning towards a version bump and just introducing ctx rather than FooContext for backwards compatibility, but bumping to v3 just for context propagation also seems like overkill. 🤔

@rayokota
Copy link
Member

rayokota commented Nov 8, 2023

Subsumed by #1099

@rayokota rayokota closed this Nov 8, 2023
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.

3 participants