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

Add gauge metrics for number of existing backups and restores #1353

Merged

Conversation

fabito
Copy link
Contributor

@fabito fabito commented Apr 5, 2019

Closes #1077
Signed-off-by: fabito fuechi@ciandt.com

@skriss
Copy link
Member

skriss commented Apr 10, 2019

So far so good. I'm thinking maybe we call these metrics backup_count and restore_count? @nrb WDYT about names?

@nrb
Copy link
Contributor

nrb commented Apr 10, 2019

I like backup_count and restore_count, but I'm concerned they don't conform to convention.

Reading https://prometheus.io/docs/practices/naming/, I wonder if total_backups or total_restores would be better. That gives us the plural form and the unit (backup/restore) in the name.

@skriss
Copy link
Member

skriss commented Apr 10, 2019

Based on that sounds like we should use backups_total / restores_total (similar to http_requests_total).

@nrb
Copy link
Contributor

nrb commented Apr 10, 2019

Good catch. I'm on board with backup_total and restore_total.

@fabito
Copy link
Contributor Author

fabito commented Apr 10, 2019

I didn't like the names I picked either. I thought about the _count suffix but found it would be more appropriate for counters.

@nrb
Copy link
Contributor

nrb commented Apr 10, 2019

For updating these, I think a a go routine for updating the counters or in the resync functions (as proposed here) would be ideal. This way, they're kept up-to-date without having to worry about changing all locations that a backup/restore could be created or deleted. That's particularly true for restores, since they can be deleted directly from the client side.

@fabito
Copy link
Contributor Author

fabito commented Apr 10, 2019

Sorry but I still can't tell which option is better/cleaner (haven't studied go routines yet).
If I take the resync functions path. I was thinking on setting the resyncFunc in NewBackupController function:

c.resyncFunc = c.resync
c.resyncPeriod = syncPeriod

And then create the resync function to set the gauge value:

func (c *backupController) resync() {
	backups, err := c.lister.List(labels.Everything())
        c.metrics.SetBackupTotal(len(backups));
}

And finally do the same for restore_total. Does it make sense ?

@fabito fabito force-pushed the gauge-for-number-of-backups-and-restores branch from 6c2bde3 to 6a95ad8 Compare April 18, 2019 04:24
Signed-off-by: fabito <fuechi@ciandt.com>
@fabito fabito force-pushed the gauge-for-number-of-backups-and-restores branch from 6a95ad8 to 475cf2a Compare April 18, 2019 04:30
@fabito fabito marked this pull request as ready for review April 18, 2019 04:35
@skriss
Copy link
Member

skriss commented Apr 19, 2019

@fabito I think this looks reasonable as-is -- it's nice and simple, no need to manage our own goroutines for metrics updates.

@skriss
Copy link
Member

skriss commented Apr 19, 2019

@carlisia or @nrb PTAL

@nrb nrb merged commit 8870281 into vmware-tanzu:master Apr 19, 2019
@skriss
Copy link
Member

skriss commented Apr 25, 2019

@fabito if you're interested in continuing to contribute to velero, I'm happy to help find new issues and provide some direction! We've really appreciated your contributions so far. Let me know.

@fabito fabito deleted the gauge-for-number-of-backups-and-restores branch April 25, 2019 05:40
@fabito
Copy link
Contributor Author

fabito commented Apr 26, 2019

Sure @skriss , point me to another issue and I'll do my best.

@skriss
Copy link
Member

skriss commented Apr 29, 2019

@fabito #1410 is a relatively small UX enhancement that would be nice to get into v1.0. It would be really nice if we could audit all of the CLI commands and check for any similar missing validation.

#1136 is another metrics request that seems reasonable to add.

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.

New gauge metrics for number of existing backups and restores
3 participants