Read metrics from file (v2) #1118
Conversation
There was a problem hiding this comment.
Hi @redblom. Thanks for the change. A few comments:
- The initDataDriver method is called for each metrics refresh. Let's refactor it so that it's called only when initializing.
- For logging please initialize the logger like this.
- The if else construct for choosing the driver won't scale. Can you follow the registry system we follow for other packages?
- The file needs to be re-read on each refresh.
|
|
redblom
force-pushed
the
read-metrics-from-file-2
branch
from
6f97b59
to
52a3616
Compare
| @@ -1,7 +1,12 @@ | |||
| [shared] | |||
| jwt_secret = "Pive-Fumkiu4" | |||
| # one of dummy, json. | |||
There was a problem hiding this comment.
These need to be defined for the service, not shared conf.
|
|
||
| // Unprotected returns all endpoints that can be queried without prior authorization. | ||
| func (s *svc) Unprotected() []string { | ||
| return []string{"/"} |
pkg/metrics/config/config.go
Outdated
| // Init sets sane defaults | ||
| func (c *Config) Init() { | ||
| if c.Prefix == "" { | ||
| c.Prefix = "metrics" |
There was a problem hiding this comment.
metrics is supposed to be the prefix for the prometheus service. This would cause conflicts.
|
|
||
| // Prefix returns the main endpoint of this service. | ||
| func (s *svc) Prefix() string { | ||
| return s.prefix |
|
|
||
| var drivers map[string]reader.Reader // map key is driver type name | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
Remove the logs. Let's keep it simple https://github.com/cs3org/reva/blob/master/pkg/ocm/invite/manager/registry/registry.go
There was a problem hiding this comment.
@ishank011 Ok, I do the loading like in that registry as well. That is better.
Do you agree with the GetDriver() method or do you want me to 'copy' https://github.com/cs3org/reva/blob/master/pkg/ocm/invite/manager/registry/registry.go ? Like exactly like that?
pkg/metrics/config/config.go
Outdated
|
|
||
| // Config holds the config options that need to be passed down to the metrics reader(driver) | ||
| type Config struct { | ||
| Prefix string `mapstructure:"prefix"` |
|
|
||
| // GetNumUsers returns the number of site users | ||
| func (d *MetricsJSONDriver) GetNumUsers() int64 { | ||
| return readJSON(d).NumUsers |
There was a problem hiding this comment.
We're reading the file each time for all the three metrics, but they would be accessed simultaneously. Can you see if we can prevent reading the file three times?
There was a problem hiding this comment.
@ishank011 Hmm, that's like the initial implementation where the whole pkg got initialized on each metrics server call and all methods where read in one go. It is my recollection that it was decided to changed it to be like this.
I have to think about this.
Maybe we can leave it for now?
redblom
force-pushed
the
read-metrics-from-file-2
branch
from
52a3616
to
473da68
Compare
@ishank011 : Metrics initializes itself, performs periodic data recording, uses sharedconfig only.
Example json metrics data file for the currently expected metrics:
{ "cs3_org_sciencemesh_site_total_num_users": 7000, "cs3_org_sciencemesh_site_total_num_groups": 550, "cs3_org_sciencemesh_site_total_amount_storage": 1080001003 }