Copilot commented on code in PR #3557: URL: https://github.com/apache/brpc/pull/3557#discussion_r4068116123
########## src/bvar/histogram.cpp: ########## @@ -0,0 +1,258 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <algorithm> // std::adjacent_find +#include <functional> // std::greater_equal +#include <iterator> // std::next +#include <string> // std::string +#include "butil/float_util.h" // butil::IsFinite +#include "butil/logging.h" // LOG +#include "butil/strings/string_number_conversions.h" // butil::DoubleToString +#include "bvar/histogram.h" + +namespace bvar { + +Histogram::BucketSchema::BucketSchema(std::initializer_list<double> bounds) + : _bounds(bounds) { + validate_bounds(); +} + +Histogram::BucketSchema::BucketSchema(const std::vector<double>& bounds) + : _bounds(bounds) { + validate_bounds(); +} + +void Histogram::BucketSchema::validate_bounds() { + for (auto it = _bounds.begin(); it != _bounds.end();) { + if (!butil::IsFinite(*it)) { + LOG(ERROR) << "Bucket bounds must be finite, dropping " << *it; + it = _bounds.erase(it); + } else { + ++it; + } + } + + // Drop the bounds that do not keep the sequence strictly ascending. + auto it = _bounds.begin(); + while ((it = std::adjacent_find(it, _bounds.end(), + std::greater_equal<>())) != _bounds.end()) { + auto bad = std::next(it); + LOG(ERROR) << "Bucket bounds must be strictly ascending, dropping " + << *bad << " which is not greater than " << *it; + _bounds.erase(bad); + } + if (_bounds.size() >= MAX_HISTOGRAM_BUCKETS) { + // Shrinking a vector drops the tail, keeping the tightest bounds. + LOG(ERROR) << "A histogram takes at most " << MAX_HISTOGRAM_BUCKETS - 1 + << " bounds, dropping the " + << _bounds.size() - (MAX_HISTOGRAM_BUCKETS - 1) + << " ones past the limit"; + _bounds.resize(MAX_HISTOGRAM_BUCKETS - 1); + } + if (_bounds.empty()) { + // A schema without any bound would send everything into the +Inf + // bucket, which no quantile can be read off. Keep the histogram + // usable rather than aborting the process over a misconfiguration. + LOG(ERROR) << "A histogram needs at least one bound, falling back to 1"; + _bounds.push_back(1.0); + } +} + +std::ostream& operator<<(std::ostream& os, const Histogram::Value& v) { + os << "{\"count\":" << v.num << ",\"sum\":" + << detail::prometheus_double_to_string(v.sum) + << ",\"counts\":["; + size_t nbuckets = std::min(v.num_buckets, MAX_HISTOGRAM_BUCKETS); + for (size_t i = 0; i < nbuckets; ++i) { + if (i != 0) { + os << ','; + } + os << v.counts[i]; + } + return os << "]}"; +} + +Histogram::Histogram(const BucketSchema& schema) + : _schema(schema) + // Both identities carry `num_buckets` so that a value combined out of no + // agent at all still knows how wide it is. + , _combiner(std::make_shared<combiner_type>(value_type(schema.num_buckets()), + value_type(schema.num_buckets()))) + , _sampler(nullptr) { +} + +Histogram::Histogram(const butil::StringPiece& name, const BucketSchema& schema) + : Histogram(schema) { + expose(name); +} + +Histogram::Histogram(const butil::StringPiece& prefix, + const butil::StringPiece& name, + const BucketSchema& schema) + : Histogram(schema) { + expose_as(prefix, name); +} + +Histogram::~Histogram() { + // Calling hide() manually is a MUST required by Variable. + hide(); + if (_sampler != nullptr) { + _sampler->destroy(); + } +} + +Histogram& Histogram::operator<<(double value) { + if (BAIDU_UNLIKELY(!butil::IsFinite(value))) { + LOG_EVERY_SECOND(WARNING) << "Ignoring non-finite value=" << value + << " recorded into Histogram(" << name() << ')'; + return *this; + } + agent_type* agent = _combiner->get_or_create_tls_agent(); + if (BAIDU_UNLIKELY(agent == nullptr)) { + LOG(FATAL) << "Fail to create agent"; + return *this; + } + // `_schema` outlives the call, the op only borrows it to find the bucket. + agent->element.modify(detail::AddSampleToHistogram(&_schema), value); + return *this; +} + +Histogram::sampler_type* Histogram::get_sampler() { + if (_sampler == nullptr) { + _sampler = new sampler_type(this); + _sampler->set_debug_name(name()); + _sampler->schedule(); + } + return _sampler; +} + +int Histogram::expose_impl(const butil::StringPiece& prefix, + const butil::StringPiece& name, + DisplayFilter display_filter) { + int rc = Variable::expose_impl(prefix, name, display_filter); + if (rc == 0 && _sampler != nullptr) { + _sampler->set_debug_name(this->name()); + } + return rc; +} + +void Histogram::describe(std::ostream& os, bool /*quote_string*/) const { + value_type v = get_value(); + os << "{\"count\":" << v.num + << ",\"sum\":" << detail::prometheus_double_to_string(v.sum) + << ",\"bounds\":["; Review Comment: `describe()` is documented and consumed as JSON, but `prometheus_double_to_string()` emits `+Inf`/`-Inf` when a finite observation sum overflows. Those bare tokens are not valid JSON, so `/vars` becomes unparsable after a legitimate overflow even though the Prometheus exporter needs this spelling. Keep Prometheus formatting confined to `dump_samples()` and serialize non-finite JSON values in a JSON-compatible form (or otherwise define the overflow representation). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
