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

Add registry for external sinks #65

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

banks
Copy link
Member

@banks banks commented Sep 5, 2017

Fixes #63

Comments welcome. I added registration to existing sub-package sinks. Happy to leave that out if there is a good reason to not auto-register those. I added some basic tests for those but have not actually tested end-to-end.

@blalor
Copy link

blalor commented Dec 11, 2018

This looks good!

go test fails because of the issue addressed in #88; I cherry-picked that PR into this branch and tests pass.

@blalor
Copy link

blalor commented Dec 11, 2018

Upon further investigation, all of the sinks need to be registered; right now that only happens if the sink's package is imported in another program. Without that, metrics.NewMetricSinkFromURL("dogstatsd://localhost:8125") will return an error.

@banks
Copy link
Member Author

banks commented Jan 7, 2019

@blalor that was the intention - if we default imported all the packages to avoid that then there is not much use having them as separate "optional" packages since they would always be pulled in even if not used, and would be enabled for any aoplication that uses this even if not desired just because the end-user could modify the URL.

Requiring the extended (or any other third-party) sink to be explicitly enabled via an anonymous import make this a backward compatible change (no new features or code magically enabled for existing applications), while allowing both the extended sinks in the repo and external ones to re-use the same setup/config via url.

@banks
Copy link
Member Author

banks commented Jan 7, 2019

I agree we should maybe document that the anonymous import is needed in the comments added here and probably in the README though...

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.

None yet

2 participants