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

Consider adding mechanism to register alternate Sinks with NewMetricSinkFromURL #63

Open
banks opened this issue Aug 30, 2017 · 2 comments

Comments

@banks
Copy link
Member

banks commented Aug 30, 2017

In my case, I'd like to use the provided sub-package Dogstatsd sink but without loosing the abstraction this library provides.

While I could write my own config/setup method that parses URL and does something different if protocol is dogstatsd:// (i.e. instantiates dogstatsd sink directly instead of using NewMetricSinkFromURL) it seems undesirable to reinvent that wheel.

Proposal

Allow external packages to register syncs early in execution (i.e. in func init()) that can hook into the central mechanism.

That might look something like this:

// Existing registry
var sinkRegistry = map[string]sinkURLFactoryFunc{
	"statsd":   NewStatsdSinkFromURL,
	"statsite": NewStatsiteSinkFromURL,
	"inmem":    NewInmemSinkFromURL,
}

// Add new mutex - might be unnecessary if we document that it's only safe to register in 
// `init` methods but I haven't found spec to say those are guaranteed not to be run in 
// parallel yet either.
var registryMu sync.RWMutex

func RegisterSyncForScheme(scheme string, factory sinkURLFactoryFunc) {
    registryMu.Lock()
    defer registryMu.Unlock()

    sinkRegistry[scheme] = factory
}

// Existing func
func NewMetricSinkFromURL(urlStr string) (MetricSink, error) {
    // ...
    registryMu.RLock()
    sinkURLFactoryFunc := sinkRegistry[u.Scheme]
    registryMu.RUnlock()
    /// ...
}

Then the included additional sinks or custom sinks defined in outer packages can use the same method to be instantiated at runtime.

I made an issue rather than just a straight PR with that code in just to check that there is no good reason this wasn't done already. If you agree it's worthwhile @armon I can make a PR.

@banks
Copy link
Member Author

banks commented Aug 30, 2017

Golang spec says:

Package initialization—variable initialization and the invocation of init functions—happens in a single goroutine, sequentially, one package at a time.

https://golang.org/ref/spec#Package_initialization

So we could go without mutex and document that it's only valid to call Register... from func init.

@armon
Copy link
Member

armon commented Sep 4, 2017

@banks I like it! Seems like a good way to make it extensible

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

No branches or pull requests

2 participants