Add back mixerclient stats #1119
Conversation
googlebot
added
the
cla: yes
src/envoy/mixer/stats.h
Outdated
| public: | ||
| MixerStatsObject(Event::Dispatcher& dispatcher, | ||
| ::google::protobuf::Duration update_interval, | ||
| GetStatsFunc func, MixerFilterStats& stats); |
There was a problem hiding this comment.
Put all these "T&" arguments first. move "stats" right after dispatcher
|
Could you do a manual stress test of Envoy with --concurrent=100 to see if it crash? |
|
Please take a look. Thanks! |
src/envoy/mixer/stats.h
Outdated
| void CheckAndUpdateStats(const ::istio::mixer_client::Statistics& new_stats); | ||
|
|
||
| // A set of Envoy stats for the number of check, quota and report calls. | ||
| MixerFilterStats stats_; |
There was a problem hiding this comment.
It is better to use &. Here you are making a copy. I am not sure of a copy of references, or a copy of the stat slots?
There was a problem hiding this comment.
It should be a reference, not a copy. Thanks!
JimmyCYJ
force-pushed
the
revert-mixerclient-stats
branch
from
b268ff8
to
421faf1
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiwzhang The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
|
hey @JimmyCYJ, thanks a lot for this - quick question: what is different between this version and the one that was crashing before ? |
|
The place allocating "stat" struct is different. "stat" is thread local data. Before it was allocated on top of another thread local data. This is the case Envoy doesn't support. Now the change is to allocate it on the Config, which is not thread local data. |
What this PR does / why we need it:Revert Mixerclient stats back.
Move Envoy stats into Mixer filter config object.
Keep MixerStatsObject in HttpMixerControl and TcpMixerControl, and pass Envoy stats into MixerStatsObject for periodical stats update.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #132 istio/old_mixerclient_repo#132Special notes for your reviewer:
Release note: