Quick drive by comment. This file is getting huge and will continue to get huger. Can you put all the new Hystrix admin stuff into a dedicated file, potentially in a new admin directory? We will need to start decomping the rest of this file also as we add more stuff to it.

moved hystrix stuff from admin.cc - commit a05aea1

Can you say a little more about why this is needed? This is a bit odd. I assume it's because you don't want chunk transfer encoding but you don't have a content-length? Is there any way to handle this without this kind of hack?

Your assumption is right. I saw that the ":" wes created exactly to allow such kind of hook and it is being used in some other places. Alternatives could be (1) changing the interface of encodeHeaders(), which is much more intrusive since it is all over, or (2) building the response without using encodeHeaders(). What do you recommend?

I'm not crazy about it, but it's probably the least bad option for doing this. If we go this route can you please add liberal comments on why this is here, that it is a non-standard header, etc.? @alyssawilk WDYT?

nit: prefer std::make_unique<Stats::Hystrix>() here and throughout (this may be common elsewhere in Envoy, but we recently moved to C++14).

Why is this class under include/? Generally, in Envoy, only pure interfaces with no implementation go under include/, and all implementations of those interfaces (even if there's just one) go under source/.

Could this be separated into a pure interface here and a default implementation under source/? Generally within Envoy, classes declared under include/ are pure interfaces with all PURE (= 0) methods. Even the default implementations are generally separated from the interface and placed under source/

fixed by commit 61659a3

As indicated in another comment thread, we can eliminate HandlerInfo completely and instead pass the AdminFilter* into each handler, which will clean up the dynamic_cast and remove a special-case from where the HandlerInfo is constructed.

It looks like as a sink you still need to get the Http::StreamDecoderFilterCallbacks* into your system, but can we get this by plumbing through the AdminFilter& rather than having this empty-base-class/dynamic-cast strategy? Or @mattklein123 do you have a better idea?

I agree - Either the Http::StreamDecoderFilterCallbacks* should be passed through or you could add some new public methods to AdminFilter to do what you want and pass it through.

nit: Why not just handler_info_ = std::make_unique<HystrixHandlerInfo>(callbacks_);?

Could you elaborate in your comment on why can't you use callbacks_->encodeData() here instead of getting at the raw connections? I'm having a little trouble understanding.

Right. Probably can use encodeData()..

nit: remove & from the capture since you don't need to capture anything from the environment - ditto in other test cases.

This is not related to this PR. Want us to fix those here anyway?

Oh, I didn't notice that you were only modifying part of that line. It'd be awesome if you could clean that up, though!

nit: generally prefer types with defined sizes - like int64_t. Also, it seems like you use int64_t as the return value for getRollingValue() which is a diff of these inputs. A similar inconsistency exists for num_of_buckets_/current_index_ and DEFAULT_NUM_OF_BUCKETS.

Why are all these being casted to doubles and then casted back to integers?

This is only half-baked, but maybe it makes sense to make all the handlers classes instead of just functions and pass them (or provide callbacks to get) certain state, like callbacks_, on each request. Then, they can manage the creation of any per-request state that may need to be created internally. That would eliminate the need to modify this function every time a handler wants to use HandlerInfo, and it would eliminate the dynamic_cast.
What do you think?

I agree, we also thought about something like that. How about opening a separate issue for that?

Yup, that seems reasonable to me. Could you also toss a TODO in there referencing the issue?

+1 to the idea of moving the bulk of the logic to a stats sink.

We thought of doing this later as an enhancement..

The reason I mentioned this was that I was thinking more about using stats sinks for this. I'd be interested to hear @mattklein123's and @htuch's thoughts. IIUC, the general architecture is a subscription mechanism where a client reaches out to subscribe and then Envoy streams specific stats data indefinitely over that connection. Is this correct?

That would mean that this is a combination of the existing pull-based integrations, like prometheus, that just implement a simple request-query-response handler on the admin port and push-based integrations, like the GRPC stats streamer and statsd, that implement stats sinks to push stats to subscribers. A lot of the custom functionality that you've added, like querying stats and periodic flushes, are already handled by the existing sink interface.

I'm wondering if there's some way that we can keep an admin port subscription model, but pass off the stream to a stats sink where the periodic pushing can be done. This would likely allow the pushing to be more performant since it won't require a new periodic query to be set up for each subscriber (assuming the multi-subscriber case would ever happen in a hystrix deployment). It would also mean that you wouldn't have to reinvent the wheel when it comes to building and configuring a push system within Envoy or querying stats data. And, as you mentioned, it will have built-in support for histograms. I'm curious to hear what others think, though.

+1, agreed. We should do this using a stat sink. I haven't through it through fully, but I don't see any reason why we can't figure out a way in which the active admin stream registers with the sink, and then when the flush calls happen it would just work? @mrice32 can you help guide this PR in that direction?

Stats sink means the stream will be sent at same intervals as other stats which are active. Isn't it a limitation?

@mattklein123, sounds good.

@trabetti, yes, it is a bit of a limitation. However, I think most deployments have only been using one sink so far, so the aligned intervals have not been an issue. If this is something you foresee being an issue, we should open an issue and design a way for stats sinks to optionally specify a time interval that is different from the global one. It shouldn't be too hard to make this more flexible.

const std::string& cluster_name, const std::string& stats.

or maybe:

absl::string_view cluster_name, absl::string_view stats

absl::StrCat (which knows how to reserve the right size for the output buffer for all inputs, before it starts appending bytes).

auto& rolling_stats_map_entry = rolling_stats_map_[key] and CSE everywhere that appears.

absl::StrCat.

const refs or string_view

Use string_view here. Also declare non-changing temps as const. Also all your temp strings below should be absl::string_view.

capture dataMessage.find(key) in a temp key_pos and EXPECT_NE(absl::string_view::npos, key_pos);

a slightly nicer pattern is to return the actual, and do the EXPECT_EQ at the call-site, which gives you useful line-numbers if it fails.

all your .find returns need to be checked. Also, please give a comment showing what the format is you are parsing.

I wonder if you could have your helper do a more complete parsing of dataMessage, returning a map<string, uint64_t> which you could then just EXPECT_EQ against a map that's initialized to the above gold values.

The existing checkStreamField looks like it might ignore stray characters in the string, and would definitely ignore extra field/value pairs. It also won't work if one of the keys is a substring of another.

IMO dynamic_cast should rarely be needed, and when used the result certainly need to be checked for nullptr (so it should be pointers not refs).

This all might be moot anyway if you move to using sinks maybe this can all be reverted.

But if we decide not to go with sinks in this PR, and you need to add this as a new stats callback, then my suggestion is to allocate Stats::HystrixHandlerInfoImpl as a temp inside a simple handler. It does look like you currently need to pass through callbacks_ to construct the HystrixHandlerInfoImpl, but since the handlers are non-static methods. you have that available to you in the callback. You don't need to change MAKE_ADMIN_HANDLER, the dispatching mechanism, or all the signatures of the existing handlers.

That would clean this up quite a bit from a code and PR-size perspective.

But really the indicates that this admin handler is really a lot different from all the other ones, and points strongly toward having an architectural discussion. In particular, this admin handler actually starts a timer and does stuff after the request terminates, which is very different semantics from all the other admin handlers, which either do a simple mutation or return some data. Probably this is another signpost pointing us toward sinks as a better solution. WDYT?

In fact, the request never terminates. We must use the same socket that was used in the original request for streaming. If the request was terminated, the socket would have been closed. I think this was the main issue with implementing as a sink. Will need some help to resolve this.

@jmarantz Totally agree with the sentiment. However, IIUC, the handlers don't have the private variables of the filter (or the filter, itself) available to them since they are methods on the AdminImpl object, but I may be missing something.

Above, we discussed above adding a handler class (rather than just a method) to combine the callback and data and formalize the idea of combining handlers with state. This would allow handlers to allocate and manage their own state, be tied to the filter object, and not require any dynamic_casting. We decided to add this as a TODO rather than do it in this PR, but maybe we should get that in as a part of this PR instead? What do you think?

Side note: as long as a bad_cast exception is the desired result of a failed dynamic_cast, I think dynamic_casting a ref should be fine since it automatically throws if the cast fails.

@trabetti I think this will still work exactly the same way as it does now in a sink. I think the request object would need to be registered with the sink, and then the sink could start encoding data through it without closing it, just as the timers do now.

I still think it should be a separate PR, but maybe it should go first, and have the current one on top of it.

@mrice32 Good point; how about supplying the AdminFilter* to the callbacks, rather than an empty HandlerInfo* base class that needs to be cast whenever it is used. That would require passing 'this' to parent_.runCallback() in AdminFilter::onComplete, and then passing that through to the handlers, so they'd have context of the filter instantiation that invoked them.

@trabetti IIUC, you're saying you want to break these into two PRs - first the sink that the handler would register with, and then add the handler, itself, in a separate PR? I think that's a great idea.

@jmarantz yup, seems reasonable to me.

@mrice32 I meant for first PR for adding a handler class instead of a method. Then, modify this PR to use the class.

Oh, I see. Yup, that would be fine too. I'm always in favor of dividing up PRs where possible :)

As indicated in another comment thread, we can eliminate HandlerInfo completely and instead pass the AdminFilter* into each handler, which will clean up the dynamic_cast and remove a special-case from where the HandlerInfo is constructed.

rolling_stats_map_[key].resize(num_o_buckets_, value) is all you need; no need for the if-statement or .find() check.

const std::string& here and in all formals.

Generally the formatter requires that you add spaces between args. You need to run the formatter under docker with every commit.

If this is going to happen a bunch, do you want to cache the stats.counter return references and store them in the class, rather than doing the name-lookup each time?

why '-' instead of '+' here? Just add comment if this was intentional.

typo "from histogram"? Start comments with a capital letter, end with a period.

for (auto it : rolling_stats_map_) {