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.

Sign up for GitHub

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

Jump to bottom

Rework StatsD usage to use separate client #594

Merged
merged 4 commits into from
Nov 7, 2019
Merged

Rework StatsD usage to use separate client #594

merged 4 commits into from
Nov 7, 2019

Conversation

wvanbergen
Copy link
Contributor

@wvanbergen wvanbergen commented Oct 18, 2019 •
edited
Loading

What are you trying to accomplish with this PR?

Get rid of the StatsD monkeypatches that cause us to be stuck on a specific version of statsd-instrument.

How is this accomplished?

By instantiating a separate StatsD::Instrument::Client, there is no need to override the singleton anymore.

What could go wrong?

Metrics may be broken after this change, although the test suite suggests otherwise.

@wvanbergen
Copy link
Contributor Author

Updated this PR. The client that gets instantiated should now reflect the legacy client we would instantiate before.

One change is that I changed STATSD_DEV=1 to STATD_ENV=development, because this is a standard environment variable that the library already supports.

@wvanbergen wvanbergen force-pushed the statsd_refactor branch 2 times, most recently from d248eac to fa94478 Compare October 21, 2019 19:54
wvanbergen
wvanbergen commented Oct 21, 2019
View reviewed changes
lib/kubernetes-deploy/deploy_task.rb Outdated
"Successfully deployed all #{@namespace} resources to #{@context}",
alert_type: "success", tags: statsd_tags << "status:success")
StatsD.distribution('all_resources.duration', StatsD.duration(start), tags: statsd_tags << "status:success")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was a (minor) bug. << modifies the array, and then returns it. This means that every time it is called, the array grows by one element. In this case, we add the same tag twice, which doesn't really cause any issues in practice, but I figured I should fix this while I am at it.

dturn
dturn approved these changes Oct 21, 2019
View reviewed changes
Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

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

Not sure I'm the best person to review this but looks reasonable. Can you please add an entry in the change log as well.

@wvanbergen
Copy link
Contributor Author

I added a changelog entry, but I still have to fix a compatibility issue in the test helpers in statsd-instrument.

timothysmith0609
timothysmith0609 previously approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me

Edit: sorry just saw the failing tests. Will rereview once those are fixed

lib/kubernetes-deploy/statsd.rb Outdated
self.backend = ::StatsD::Instrument::Backends::NullBackend.new
def self.client
@client ||= begin
sink = if ::StatsD::Instrument::Environment.current.env.fetch('STATSD_ENV', nil) == 'development'
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems as though the ::StatsD::Instrument::Environment.current.env is just a basic wrapper around ENV, in this case. The other fallbacks appear to be geared more towards Rails applications, of which krane isn't one. With that in mind, it might be cleaner just to do if ENV.fetch(X, nil) == Y, instead.

@timothysmith0609 timothysmith0609 dismissed their stale review October 22, 2019 13:08

Tests are still failing

@wvanbergen
Copy link
Contributor Author

Shopify/statsd-instrument#234 fixes the issue that causes us not being able to parse and assert stuff on the event datagrams being emitted.

@wvanbergen @dturn
parent 50e7217
0e1605f 
author Willem van Bergen <willem@vanbergen.org> 1571413339 -0400
committer Daniel Turner <daniel.turner@shopify.com> 1573059976 -0800

Rework StatsD usage to use separate client

Update StatsD client initialization to resemble how we instantiated a client before.

Add CHANGELOG entry.
wvanbergen
wvanbergen commented Nov 6, 2019
View reviewed changes
Copy link
Contributor Author

@wvanbergen wvanbergen left a comment

Choose a reason for hiding this comment

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

LGTM

timothysmith0609
timothysmith0609 approved these changes Nov 6, 2019
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
dturn and others added 2 commits November 6, 2019 10:54
@dturn @timothysmith0609
Update CHANGELOG.md
a88aedf 
Co-Authored-By: Timothy Smith <31742287+timothysmith0609@users.noreply.github.com>
@dturn
Merge branch 'master' into statsd_refactor
81768e3
@dturn dturn merged commit d466f02 into master Nov 7, 2019
@dturn dturn deleted the statsd_refactor branch November 7, 2019 00:30
@dturn dturn mentioned this pull request Nov 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timothysmith0609 timothysmith0609 timothysmith0609 approved these changes

@dturn dturn dturn approved these changes

@dalehamel dalehamel Awaiting requested review from dalehamel

@KnVerey KnVerey Awaiting requested review from KnVerey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wvanbergen @dturn @timothysmith0609