-
Notifications
You must be signed in to change notification settings - Fork 112
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
New metrics package #740
New metrics package #740
Conversation
@redblom can you confirm that you can obtain the metrics by doing |
Yes, but only if I let the prometheus service New() function return a promhttp.Handler() like so: |
@redblom interesting, let me take a look |
@@ -31,6 +33,7 @@ import ( | |||
|
|||
func init() { | |||
global.Register("prometheus", New) | |||
fmt.Printf("metrics - %v \n", metrics.NumUsersMeasure.Description()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove this debug line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yes :)
@redblom thanks for the update, only one comment and I can merge |
"net/http" | ||
"reva/pkg/metrics" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to refer to the github repo path, I guess that's why the build isn't passing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed that, good catch-up @ishank011
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ishank011 How to resolve this? Exclude prometheus.go modifications from this pull request, and make that a separate pull request after this one is merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just change it to github.com/cs3org/reva/pkg/metrics
. Since the remote origin points to the repo itself, it'll resolve it itself.
pkg/metrics/metrics.go
Outdated
var ( | ||
NumUsersMeasure = stats.Int64("cs3_org_sciencemesh_site_total_num_users", "The total number of users within this site", stats.UnitDimensionless) | ||
NumGroupsMeasure = stats.Int64("cs3_org_sciencemesh_site_total_num_groups", "The total number of groups within this site", stats.UnitDimensionless) | ||
AmountStorageMeasure = stats.Int64("cs3_org_sciencemesh_site_total_amount_storage", "The total amount of storage used within this site", stats.UnitDimensionless) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use stats.UnitBytes here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe only on storage amount ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, just on storage. Github shows the whole blob even if you comment on a single line.
pkg/metrics/metrics.go
Outdated
// here we must request the actual number of site users | ||
// for now this is a mockup: a number increasing over time | ||
numUsersCounter += int64(rand.Intn(100)) | ||
fmt.Printf("nrUsers = %v \n", numUsersCounter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output to stdout isn't logged, so I guess we can remove these as well
@@ -53,6 +56,13 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) | |||
|
|||
view.RegisterExporter(pe) | |||
|
|||
// register the desired measures' views | |||
view.Register( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the result of Register as done here and return if there's an error.
@labkode @ishank011 all comments looked after. New commits pushed. |
3 site metrics for now: number of users, number of groups, storage amount in use