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 server-side metrics #236

Merged
merged 5 commits into from
Mar 19, 2021
Merged

Add server-side metrics #236

merged 5 commits into from
Mar 19, 2021

Conversation

thao-wish
Copy link
Contributor

Add server-side metrics #234

Signed-off-by: Jia Hao <thao@wish.com>
Signed-off-by: Jia Hao <thao@wish.com>
@mattklein123 mattklein123 self-assigned this Mar 12, 2021
name: "ratelimit_service_total_requests"
match_metric_type: counter
- match: "ratelimit_server.response_time"
name: "ratelimit_service_response_time_seconds"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s.responseTime.AddValue(float64(time.Since(start).Milliseconds())) is this in milliseconds or seconds ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the statsd exporter statsd format is milliseconds and prometheus format is seconds, so the exporter will convert the timer value to seconds.

Signed-off-by: Jia Hao <thao@wish.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks looks good to me at a high level. Please add tests.

/wait

Signed-off-by: Jia Hao <thao@wish.com>
Signed-off-by: Jia Hao <thao@wish.com>
func newServerMetrics(scope stats.Scope, fullMethod string) *serverMetrics {
_, methodName := splitMethodName(fullMethod)
ret := serverMetrics{}
ret.totalRequests = scope.NewCounter(methodName + ".total_requests")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the existing code tends to use metric names rather than tags, but for what its worth: I'd much rather see methodName as a tag rather than part of the metric name. (More generally, it seems like there's little to no reason a metric name ever needs to be dynamically created in a system that supports metrics with tags)

Using tags makes it simple in most metric systems to aggregate stats across method names or break down to specific subsets of method names.

Aggregating across metric names, by contrast, is not particularly simple or normal or standard in most metrics systems. Using different metric names also makes discovering what metric exist by looking at metric names a mess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that metrics tags are always the better choice than embedding in metrics name, but I don't think prom-statsd exporter supports the tagging format used by gostats. (please correct me if I'm wrong) So we'll have to do regexp matching to export it as a prom label, which is slower and more work than using glob patterns.

If there is a quick and easy way to export tags as prom labels, I'm happy to use tags instead.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit 830d433 into envoyproxy:main Mar 19, 2021
zdmytriv pushed a commit to verygoodsecurity/ratelimit that referenced this pull request Aug 2, 2021
Signed-off-by: Jia Hao <thao@wish.com>
timcovar pushed a commit to goatapp/ratelimit that referenced this pull request Jan 16, 2024
Signed-off-by: Jia Hao <thao@wish.com>
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.

4 participants