-
Notifications
You must be signed in to change notification settings - Fork 290
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
Remove global stats variables for uhttp #318
Conversation
@peter-edge, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yutongp, @anuptalwalkar and @madhuravi to be potential reviewers. |
func SetupHTTPMetrics(scope tally.Scope) { | ||
httpScope := scope.Tagged(HTTPTags) | ||
HTTPPanicCounter = httpScope.Counter("panic") | ||
// Client is a client for stats. |
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.
Nit: I think metrics were moved to internal package, because of the globals, now they can become private structs in the uhttp package.
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.
Ugh haha ok, I'll do that
modules/uhttp/stats.go
Outdated
@@ -33,31 +33,46 @@ const ( | |||
TagMiddleware = "middleware" |
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.
Shall these tag names be private?
modules/uhttp/stats.go
Outdated
@@ -18,7 +18,7 @@ | |||
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | |||
// THE SOFTWARE. | |||
|
|||
package stats | |||
package uhttp |
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.
since we are moving stats
back to uhttp
, how about client
? ;P
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.
Another PR I think :)
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.
lgtm - of course, please get stamps from 'ze UberFX team :)
No description provided.