Skip to content

statsd cleanup#778

Merged
jnunemaker merged 1 commit intomainfrom
statsd-cleanup
Dec 2, 2023
Merged

statsd cleanup#778
jnunemaker merged 1 commit intomainfrom
statsd-cleanup

Conversation

@dpep
Copy link
Copy Markdown
Collaborator

@dpep dpep commented Dec 2, 2023

I was poking around and discovered the statsd subscriber. In addition to being excited to test it out in prod, I noticed a couple small improvements we might make to simplify - using the &. operator to guard against nil objects, and adding test coverage... what'd you think?

@jnunemaker
Copy link
Copy Markdown
Collaborator

Hey thanks!

Do you think we should warn or raise when the client is not set? I’m wondering if it would be confusing to have it fail silently.

@dpep
Copy link
Copy Markdown
Collaborator Author

dpep commented Dec 2, 2023

yeah, if someone explicitly plugs in the statsd subscriber but doesn't provide a client, failing loudly makes sense to me. I just didn't want to change the functionality on my first pass...

we could also expose something in the config dsl that makes setup easier, eg.

Flipper.configure do |conf|
  conf.statsd = my_client
end

@jnunemaker jnunemaker merged commit 134b111 into main Dec 2, 2023
@jnunemaker jnunemaker deleted the statsd-cleanup branch December 2, 2023 20:28
@dpep dpep mentioned this pull request Dec 4, 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.

2 participants