juju-{os}-machines metric #7228

Merged
merged 1 commit into from Apr 12, 2017

Conversation

Projects
None yet
3 participants
Owner

cmars commented Apr 12, 2017

Description of change

Why is this change needed?

To count the number of non-container machines per operating system in a model.

QA steps

How do we verify that the change works?

Bootstrap a controller, add a model, add some machines to that model. juju metrics --all should show metrics, such that:

juju-machines is the total count of non-container machines in the model.
juju-ubuntu-machines is the count of non-container Ubuntu machines in the model.
juju-windows-machines is the count of non-container Windows machines in the model.
juju-centos-machines is the count of non-container CentOS machines in the model.

Documentation changes

Does it affect current user workflow? CLI? API?

No.

Bug reference

Does this change fix a bug? Please add a link to it.

N/A

@cmars cmars changed the base branch from staging to develop Apr 12, 2017

Owner

cmars commented Apr 12, 2017

!!build!!

Can we address the "unknown" OS type count before landing?

for _, machine := range allMachines {
ct := machine.ContainerType()
if ct == instance.NONE || ct == "" {
machineCount++
+ if osType, err := series.GetOSFromSeries(machine.Series()); err != nil {
+ logger.Warningf("failed to resolve OS name for series %q: %v", machine.Series(), err)
@wallyworld

wallyworld Apr 12, 2017

Owner

IMO we should count these as "unknown OS".
We can still show the tally to the user. Otherwise the numbers won't add up.

+ if osType, err := series.GetOSFromSeries(machine.Series()); err != nil {
+ logger.Warningf("failed to resolve OS name for series %q: %v", machine.Series(), err)
+ } else {
+ n := osMachineCount[osType]
@wallyworld

wallyworld Apr 12, 2017

Owner

can we make this one line?
n := osMachineCount[osType] + 1

Owner

cmars commented Apr 12, 2017

$$merge$$

Contributor

jujubot commented Apr 12, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Apr 12, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10662

Owner

cmars commented Apr 12, 2017

$$merge$$

Contributor

jujubot commented Apr 12, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit b9d19ca into juju:develop Apr 12, 2017

1 check failed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment