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

From DavidB #2

Closed
avivl opened this issue Mar 29, 2018 · 0 comments
Closed

From DavidB #2

avivl opened this issue Mar 29, 2018 · 0 comments
Labels
enhancement New feature or request

Comments

@avivl
Copy link
Contributor

avivl commented Mar 29, 2018

went over it, looks ok but I miss comments so don't really understand what many parts of code do without looking at the import packages first. For example, g.Add that uses oklog which i don't know what it is and so on. So from the main func code i could hardly understand what it does, especially with things like 'logger.Info("exit", zap.Error(g.Run()))'. I think g.Run() should be more explicit because it's your main loop and my eye automatically skips log lines. Also pls highlight somewhere what the endpoints are (/track and /metrics), unless you have a dedicated client in which case it's not that important.
collector looks fine, needs more comments though. Things like 'if counter != 7' need to be more clear
Also I'd do some wrapper around tag, for example instead of tag.NewKey("banias/keys/code") it would be mytag.Key("code") and instead of "banias/measures/request_latency" it would be mytag.Measure("request_latency") because 'banias' is constant and keys/measures/etc are categories

@avivl avivl added the enhancement New feature or request label Apr 2, 2018
@avivl avivl closed this as completed in c6488aa Apr 2, 2018
avivl added a commit that referenced this issue Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant