Add StoreOption/WithSinkWrap to expose the sink NewDefaultStore builds - #178
Open
th0114nd wants to merge 3 commits into
Open
Add StoreOption/WithSinkWrap to expose the sink NewDefaultStore builds#178th0114nd wants to merge 3 commits into
th0114nd wants to merge 3 commits into
Conversation
NewDefaultStore builds its Sink (a TCP statsd sink, or a logging/null sink depending on Settings) entirely internally and returns only the finished Store, so a caller who wants to decorate that sink -- e.g. to filter which stats actually get written -- has to reimplement NewDefaultStore's sink-selection logic just to get a handle on the sink to wrap. NewDefaultStoreWithSink(wrap func(FlushableSink) FlushableSink) Store adds that hook: it selects the sink exactly as NewDefaultStore does, but passes it through wrap before constructing the Store. NewDefaultStore itself now just calls this with an identity wrap, so there's a single implementation of the sink-selection logic instead of two.
th0114nd
changed the title
Match the existing SinkOption pattern (WithLogger, WithStatsdHost, etc. in net_sink.go) instead of introducing a separate NewDefaultStoreWithSink function: NewDefaultStore now takes variadic StoreOptions, and WithSinkWrap(wrap func(FlushableSink) FlushableSink) StoreOption lets a caller decorate the sink NewDefaultStore would otherwise have used unwrapped. NewDefaultStore() with no options is unchanged -- variadic options mean every existing call site keeps compiling and behaving identically.
storeOptions is unexported either way, so the interface+apply indirection (mirroring SinkOption) bought no extra encapsulation: nothing outside the package could construct a StoreOption regardless, since only functions in this package can reference storeOptions to write one. A plain type StoreOption func(*storeOptions) is equivalent and simpler.
th0114nd
marked this pull request as ready for review
July 28, 2026 15:51
sokada1221
reviewed
Aug 3, 2026
|
|
||
| // NewDefaultStore returns a Store with a TCP statsd sink, and a running flush timer. | ||
| func NewDefaultStore() Store { | ||
| func NewDefaultStore(opts ...StoreOption) Store { |
There was a problem hiding this comment.
I think we should add sth like NewDefaultStoreWithOptions instead, and keep the backward compatibility for NewDefaultStore - especially because this is a public library.
There was a problem hiding this comment.
brainstormed a bit more - maybe it's simpler to expose sink instead? e.g. NewDefaultSink() with settings
then pseudocode integration for matching would be
sink := Wrap(stats.NewDefaultSink())
store := stats.NewStore(sink, false)
go store.Start(time.NewTicker(settings.FlushInterval()))
...
| loggingSinkDisabled string | ||
| assertType func(t *testing.T, got FlushableSink) | ||
| }{ | ||
| {"statsd", "true", "false", func(t *testing.T, got FlushableSink) { |
There was a problem hiding this comment.
i could be wrong but doesn't this setup the full sink that tries to establish a TCP connection?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NewDefaultStorebuilds itsSink(TCP statsd, or logging/null depending onSettings) entirely internally and returns only the finishedStore-- there's no way to decorate that sink (e.g. to filter which stats are actually written) without reimplementingNewDefaultStore's sink-selection logic from scratch.NewDefaultStore, in the same spirit as the existingSinkOptionpattern for sinks (WithLogger,WithStatsdHost, etc. innet_sink.go), but as a plain func type rather than an interface:type StoreOption func(*storeOptions).storeOptionsis unexported, so the interface+apply-method indirectionSinkOptionuses wouldn't add any real encapsulation here -- callers still can't construct aStoreOptionthemselves either way.NewDefaultStore(opts ...StoreOption)andWithSinkWrap(wrap func(FlushableSink) FlushableSink) StoreOption.wrapreceives the sinkNewDefaultStorewould otherwise have used unwrapped, and its return value is what theStoreis actually constructed with.NewDefaultStore()(no options) is unchanged -- variadic options mean every existing call site keeps compiling and behaving identically. This is purely additive.Motivated by a downstream service (matching) that currently duplicates this exact sink-selection logic in its own repo just to wrap the sink in a stats-blocklist filter -- see the internal
statsblocklist.Sinkpackage for context. Opened as a draft for early feedback on the API shape before I write more tests / polish docs.Test plan
go build ./...go vet ./...go test -race ./...(128 passing, including new cases forWithSinkWrapand a zero-optionsNewDefaultStore()regression check)