nit: can you use longer names for the templated types? It's not immediately obvious what those represent.

nit: can you make the class-name match the filename? In this case I'd rename the file stats_request_base.h.

actually, after reading more of the code in the PR, I think it might be better to call the base-class StatsRequest and leave it in the same files (stats_request.h, stats_request.cc) and have the two derived types calls: GroupedStatsRequest and UngroupedStatsRequest? I think that's the essence of why you wanted them templated.

I could see that it might be desirable to have a grouped view like for Prom in HTML (just for human-friendly viewing) or in JSON for processing by another tagged system.

By leaving the bulk of the implementation in stats_request.cc and .h it might be easier to see the changes in review. The UngroupedStatsRequest would be relatively small.

WDYT?

This makes sense, I will do as you suggest 👍

can you add some class comments describing why we are templating this class?

I think it doesn't save anything to make this a short; maybe make it a uint32_t, and call it phase_index_?

Moreover, why did you elect to use an index here? I feel like it might be more error-prone to refer to these by index (noting the commit you just made changing a 2 to a 3).

I think it's not too hard in Prom to skip the text-readouts phase if that's not enabled.

The index doesn't have to do with skipping the text-readouts per se. It's used to keep track of the current phase in the shared base class, and move on to the next phase w/o having to know what that phase is.

This is because the prom and non-prom algorithms go through phases in a different order. I hope that makes sense: if you can think of a better way to achieve something similar, I am happy to update the code. For now I will rename the field name and type.

nit: you can save this per-stat lookup by saving the symbol table as a class member variable, since you are already capturing [this]

you can make this a little faster (just one hash lookup rather than 3 in some cases) with:

StatOrScopes& variant = stat_map_[tag_extracted_name];
if (variant.index() == absl::variant_npos) {
  variant = std::vector<Stats::RefcountPtr<StatType>>({stat});
} else {
  absl::get<std::vector<Stats::RefcountPtr<StatType>>>(variant).emplace_back(stat);
}

rename this saveMatchingStat? I see the original code used a local lambda check_stat for this, but that was mis-named as it's doing more than just checking. Might as well fix it now.

I'm wondering if filters are ever used with Prom. It seems weird now that I think of it to filter on the fully elaborated name, but I guess that's easier from a coding and usage perspective than filtering on the tag-extracted name and then having a separate mechanism for filtering on the extracted tags.

nit: absl::StrCat is probably faster for this use-case.

can you leave a TODO in here that support for the 3 "bucket modes" offered in other formats hasn't been ported to Prom yet?

See Utility::HistogramBucketsMode in source/server/admin/utils.h

Thanks!

sure thing!

why this change?

hmm not sure what I did here: I will revert to original.

I'm a little surprised that this if-statement is not needed. What puts the ScopeVec into the variant?

I'm just curious -- tests would obviously catch this if it didn't work.

It's the operator[] of the btree_map that runs a try_emplace behind the scenes and so inserts the variant into the map (and instantiates it using the variant's default constructor) when the lookup key is not in the map.

So for this reason the condition of the if statement is never true and the whole if block is redundant.

cool thank you! Can you add that as a code-comment here in place of the code?

scan you fix my typo? s/groups/grouped/

thanks