(POOLER-134) Ship VM usage stats#306
Conversation
mattkirby
force-pushed
the
pooler_134
branch
2 times, most recently
from
8a14bbc to
8110d1a
Compare
| user = $redis.hget("vmpooler__vm__#{vm}", 'token:user') || 'unauthenticated' | ||
| poolname = $redis.hget("vmpooler__vm__#{vm}", "template") | ||
|
|
||
| if jenkins_build_url |
There was a problem hiding this comment.
I like that you're using guard clauses above to get out of situations where we shouldn't continue with the method. Another benefit of them is that they can keep the method "flatter" (I mean there's less nesting or indents).
I wouldn't hold this PR up for this, but if you're going to come in for changes anyway, this statement could be made into a guard clause like so:
return $metrics.increment("usage.#{user}.#{poolname}") unless jenkins_build_urlI have a width-preference that would make me do that in two lines 🤷♂️ 🤷♀️, but you can see that this takes out one of the indents for most of the method since it's inside this if-block.
There was a problem hiding this comment.
Thanks. I've changed this to consider your suggestion and ship the simple metric if jenkins_build_url does not exist, and then explicitly return. The rest of the code gets to leave the block as a result.
| branch = value_stream_parts.pop | ||
| project = value_stream_parts.shift | ||
| job_name = value_stream_parts.join('_') | ||
| build_metadata_parts = url_parts[3] |
There was a problem hiding this comment.
I think this could be nil if the job is not a matrix. Then we will hit a NilClass error when trying to split it on line 368
labels_string_parts = labels_string.split(',')
The tests are great, please add one for a job that doesnt have that last part eg https://host/job/jobname/ or does ABS alway also include the build number like https://host/job/jobname/220 in which case I guess we wouldn't get the nil error but rather parse 220 in component_to_test()
That being said we should also think about error handling. Probably errors thrown by this whole method could be handled instead of throwing a stacktrace or an error in the log if this is wrapped. I would rather have us ignore the error and keep whatever processing the destroy should be doing (is there much left in that logic path?)
There was a problem hiding this comment.
This is the second to last step in the destroy. The only other thing that happens is the mutex is dereferenced so it can be removed during garbage collection. It probably is worth capturing any failures and preventing them from bubbling up, and perhaps adding a log message for when parsing the URL fails.
There was a problem hiding this comment.
I added a test to cover a jenkins job URL with no matrix information. I think you're correct that the build number ends up getting passed instead of nil. In any case, I added test coverage for both examples so we should be able to handle these cases without an error.
This commit updates vmpooler to ship VM usage stats when a VM is destroyed. The stats are gathered for jobs based on user and pool name. If a jenkins build URL is present then this is broken down by user, instance, value stream, branch and project. Additionally, if present then the RMM_COMPONENT_TO_TEST_NAME will be listed after project. Without this change we do not collect stats on per VM usage and its correlation to users and pools.
This commit updates providers_spec so the test ensures array content of the providers match. Without this change the provider_spec test will fail when comparing providers if the order is not exactly the same in each array.
4 participants
This commit updates vmpooler to ship VM usage stats when a VM is destroyed. The stats are gathered for jobs based on user and pool name. If a jenkins build URL is present then this is broken down by user, instance, value stream, branch and project. Additionally, if present then the RMM_COMPONENT_TO_TEST_NAME will be listed after project. Without this change we do not collect stats on per VM usage and its correlation to users and pools.