Report Ruby VM probe metrics as gauges with a hostname tag #866
Conversation
This file contains 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
The problem the distribution values is that they will always be the same because we report the metrics at very regular intervals, so the values between mean, 90th percentile and 95 percentile won't be any different as we've seen in the graphs these metrics create.
Call a private method that calls `Appsignal.set_guage` so that if we want to change some defaults in metrics reported by this probe we only have to do it in one place. Like adding a hostname tag.
The added hostname tag allows us to tell multiple hosts from each other and graph them separately. If we do not, an app with multiple hosts would have hosts overwrite one another's metrics. Only the last reported value would be stored and shown. I've copied some of the Sidekiq probe's and Puma plugin to determine the hostname automatically and listen to the AppSignal config in case a hostname was configured.
tombruijn
added a commit
to appsignal/public_config
that referenced
this pull request
Jul 28, 2022
We've noticed that distributions are not a good fit for this. For more PR appsignal/appsignal-ruby#866 on the Ruby gem.
luismiramirez
approved these changes
Jul 28, 2022
unflxw
approved these changes
Jul 28, 2022
| key, value, tags = distribution_value | ||
| next unless key == expected_key | ||
| next unless expected_value ? expected_value == value : !value.nil? | ||
| next unless tags == expected_tags |
There was a problem hiding this comment.
next unless ...? Shouldn't this be return false unless ...?
I suppose it doesn't make a difference -- next gives nil, which is falsy anyway. I've never seen it used this way before.
There was a problem hiding this comment.
If you call return the loop will exit and no other values will be checked. If you want it to exit out of that loop iteration with false you need to call next false if ... which is the same as next if ... for this because nil is falsy like you say.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Change all Ruby VM distributions to gauges
The problem the distribution values is that they will always be the same
because we report the metrics at very regular intervals, so the values
between mean, 90th percentile and 95 percentile won't be any different
as we've seen in the graphs these metrics create.
Refactor gauge calls to helper method in MRI probe
Call a private method that calls
Appsignal.set_guageso that if wewant to change some defaults in metrics reported by this probe we only
have to do it in one place. Like adding a hostname tag.
Report hostname tag for Ruby VM probe metrics
The added hostname tag allows us to tell multiple hosts from each other
and graph them separately. If we do not, an app with multiple hosts
would have hosts overwrite one another's metrics. Only the last reported
value would be stored and shown.
I've copied some of the Sidekiq probe's and Puma plugin to determine the
hostname automatically and listen to the AppSignal config in case a
hostname was configured.