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

remove need for nil checks around statsd metrics #932

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

jmhodges
Copy link
Contributor

@jmhodges jmhodges commented Jul 30, 2024

Currently, we have to check if autographer.stats is nil anywhere we
want to emit a statsd metric. This is error-prone and verbose. Work for
AUT-150 (and more) would be easier if we do this here.

This patch makes the autographer.stats field always non-nil. By
default, autographers created with newAutographer will have a
statsd.NoOpClient object for its stats and if the autograph config
has a statsd server configured, it will be replaced with a real statsd
client when autographer.addStats is called. That addStats call will
now always occur in autograph's main function.

This moves the conditional for checking if the statsd server is
configured into autographer.addStats. However, it would probably be
better in the future for us to have this and the similar
autograph.add* methods moved into the newAutographer contructor.
This, though, at least makes the autographer type a lil safer on
construction.

This means that we use statds.ClientInterface instead of
*statsd.Client everywhere to fit both client types.

Along the way, we also have to fix up the main_test.go that was
reproducing that conditional from the main inside it. We also fix an
signer/xpi test that was depending on a statsd server running but not
actually needing it to finish.

This patch doesn't remove all of the nil checks. A follow up will be
made to do so.

Updates AUT-159
Updates AUT-150

@jmhodges jmhodges force-pushed the statsd-null-object branch from 7659ff5 to 25b0a27 Compare July 30, 2024 20:15
@jmhodges jmhodges changed the title statsd null object remove need for nil checks around statsd metrics Jul 30, 2024
@jmhodges jmhodges force-pushed the statsd-null-object branch from 25b0a27 to a88ed97 Compare July 30, 2024 20:18
@jmhodges jmhodges marked this pull request as ready for review July 30, 2024 20:29
@jmhodges jmhodges requested review from a team as code owners July 30, 2024 20:29
@jmhodges jmhodges requested review from oskirby, eviljeff and hneiva and removed request for a team July 30, 2024 20:29
Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

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

This is not a problem with this patch, but this makes me think that we might want to add some sort of check or safeguard that ensures we don't run without statsd in production (and perhaps staging?), which I can't imagine we would ever want to do...

@jmhodges
Copy link
Contributor Author

This is not a problem with this patch, but this makes me think that we might want to add some sort of check or safeguard that ensures we don't run without statsd in production (and perhaps staging?), which I can't imagine we would ever want to do...

We'd hopefully find out because our alerts would fire with a "not getting data" alarm. Def the next step is to make sure we have alerts in place using this data!

Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

r+ for xpi/* changes, thanks

Currently, we have to check if `autographer.stats` is nil anywhere we
want to emit a statsd metric. This is error-prone and verbose. Work for
AUT-150 (and more) would be easier if we do this here.

This patch makes the `autographer.stats` field always non-nil. By
default, `autographer`s created with `newAutographer` will have a
`statsd.NoOpClient` object for its `stats` and if the autograph config
has a statsd server configured, it will be replaced with a real statsd
client when `autographer.addStats` is called. That addStats call will
now always occur in autograph's main function.

This moves the conditional for checking if the statsd server is
configured into autographer.addStats. However, it would probably be
better in the future for us to have this and the similar
`autograph.add*` methods moved into the `newAutographer` contructor.
This, though, at least makes the autographer type a lil safer on
construction.

This means that we use `statds.ClientInterface` instead of
`*statsd.Client` everywhere to fit both client types.

Along the way, we also have to fix up the main_test.go that was
reproducing that conditional from the main inside it. We also fix an
signer/xpi test that was depending on a statsd server running but not
actually needing it to finish.

This patch doesn't remove all of the nil checks. A follow up will be
made to do so.

Updates AUT-159
Updates AUT-150
@jmhodges jmhodges force-pushed the statsd-null-object branch from 934e070 to 4260381 Compare August 1, 2024 01:40
@jmhodges jmhodges merged commit 5f97ba3 into main Aug 1, 2024
15 checks passed
@willdurand willdurand deleted the statsd-null-object branch August 1, 2024 12:27
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.

3 participants