Skip to content

redis: add latency stats for commands#5593

Merged
mattklein123 merged 16 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/redis_latency_stats
Jan 23, 2019
Merged

redis: add latency stats for commands#5593
mattklein123 merged 16 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/redis_latency_stats

Conversation

@ramaraochavali
Copy link
Contributor

@ramaraochavali ramaraochavali commented Jan 14, 2019

Description: Adds latency stats for each command.
Risk Level: Low
Testing: Added automated tests
Docs Changes: Updated
Release Notes: Added
Fixes #5591

@ramaraochavali
add success and error stats for redis
0bf635e 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Merge branch 'master' into fix/redis_stats
aa7bcd9 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
reorder version history
cc9b971 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
resolve conflicts
c678903 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
add latency stats
d79d513 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
resolve conflicts
5c61416 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
formatting
8c9eee7 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
add linespace
3d24fb6 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
fix version history
bd98c71 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
fix compile error
afa5aa8 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
format
bd2da06 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

ramaraochavali commented Jan 16, 2019

@danielhochman can you PTAL when you get chance?

HenryYYang
HenryYYang reviewed Jan 18, 2019
View reviewed changes
source/extensions/filters/network/redis_proxy/command_splitter_impl.cc Outdated
} else {
command_stats_.error_.inc();
}
std::chrono::milliseconds latency = std::chrono::duration_cast<std::chrono::milliseconds>(
Copy link
Contributor

@HenryYYang HenryYYang Jan 18, 2019

Choose a reason for hiding this comment

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

I see that there's a Timespan class in Envoy.Stats that does the same thing. Maybe we should use that so that it's more consistent.

Copy link
Contributor Author

@ramaraochavali ramaraochavali Jan 18, 2019

Choose a reason for hiding this comment

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

OK. Changed to use Timespan class. PTAL.

@ramaraochavali
move to timespan
999a3c1 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Merge branch 'master' into fix/redis_latency_stats
d1e9f23 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
formatting
420773f 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

ramaraochavali commented Jan 22, 2019

@HenryYYang @danielhochman can you PTAL when you get chance?

@HenryYYang
Copy link
Contributor

HenryYYang commented Jan 22, 2019 •
edited
Loading

+1. Looks good to me.

HenryYYang
HenryYYang previously approved these changes Jan 22, 2019
View reviewed changes
@mattklein123 mattklein123 self-assigned this Jan 23, 2019
mattklein123
mattklein123 requested changes Jan 23, 2019
View reviewed changes
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.

LGTM, one small request. Thank you!

/wait

source/extensions/filters/network/redis_proxy/command_splitter_impl.h Outdated
Stats::Counter& total_;
Stats::Counter& success_;
Stats::Counter& error_;
Stats::Histogram& latency_;
Copy link
Member

@mattklein123 mattklein123 Jan 23, 2019

Choose a reason for hiding this comment

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

Can you potentially switch this over to using the stats macros so its consistent with everywhere else? Sorry I thought about asking this on the previous PR, but if we keep adding stats it's probably worth it to do so now.

Copy link
Contributor Author

@ramaraochavali ramaraochavali Jan 23, 2019

Choose a reason for hiding this comment

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

No problem. Switched over to macros. PTAL

@ramaraochavali
use stat macros
bf8bd38 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

ramaraochavali commented Jan 23, 2019

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Jan 23, 2019

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5593 (comment) was created by @ramaraochavali.

see: more, trace.

@ramaraochavali
make string const
13a6b13 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
mattklein123
mattklein123 approved these changes Jan 23, 2019
View reviewed changes
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.

Thank you!

@mattklein123 mattklein123 merged commit 18a57f2 into envoyproxy:master Jan 23, 2019
@ramaraochavali ramaraochavali deleted the fix/redis_latency_stats branch January 23, 2019 16:44
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Jan 24, 2019
@ramaraochavali @mattklein123
redis: add latency stats for commands (envoyproxy#5593)
c7e3fd5 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Signed-off-by: Dan Zhang <danzh@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Jan 31, 2019
@ramaraochavali @danzh1989
redis: add latency stats for commands (envoyproxy#5593)
9948d5a 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Signed-off-by: Rama Chavali <ramaraochavali@gmail.com>
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
@ramaraochavali @fredlas
redis: add latency stats for commands (envoyproxy#5593)
2eab6fc 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+1 more reviewer

@HenryYYang HenryYYang HenryYYang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@danielhochman danielhochman

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ramaraochavali @HenryYYang @mattklein123 @danielhochman