Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19133 )
Change subject: [metrics] Expose Prometheus Metrics in Kudu ...................................................................... Patch Set 6: (25 comments) http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@16 PS6, Line 16: accroding according http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@17 PS6, Line 17: nit: extra space http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@32 PS6, Line 32: nit: extra space http://gerrit.cloudera.org:8080/#/c/19133/6//COMMIT_MSG@34 PS6, Line 34: This also makes it possible to keep : time based units more accurate as Milliseconds, Nanoseconds and etc. : do not need to be converted to seconds which is the base time unit in Prometheus. Is it possible to operate with such metrics as time-based ones then in Graphana and other tools? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/server/default_path_handlers.cc@544 PS6, Line 544: bool not_styled = false; : bool is_on_nav_bar = true; If using variables instead of direct constants for arguments, could these be constant/constexpr? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/CMakeLists.txt@224 PS6, Line 224: prometheuswriter nit: prometheus_writer.cc http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.h@587 PS6, Line 587: std::stringstream* Why not to pass the prefix as a constant string? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@75 PS6, Line 75: const auto& val nit: could use structed binding (yes, it's C++17): https://en.cppreference.com/w/cpp/language/structured_binding http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@413 PS6, Line 413: Status s = GetMetricsAndAttrs(filters, &metrics, &attrs); What if this returns non-OK status? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@415 PS6, Line 415: if (strcmp(prototype_->name(),"server") == 0) { nit for here and below: add space between next argument and the comma. http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@416 PS6, Line 416: strcmp(id_.c_str(),"kudu.master") == 0 Here and below: why to convert into C string for comparison? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@418 PS6, Line 418: std::stringstream prefix; : prefix << "kudu_master_"; stringstream is a heavy and expensive object; use std::string or just const char* instead http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@431 PS6, Line 431: return Status::OK(); What if prototype_->name() isn't "server" -- why to return Status::OK() in that case if nothing has been written? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@590 PS6, Line 590: nit: the indent is messed up http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@733 PS6, Line 733: std::stringstream Why not just std::ostringstream? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@907 PS6, Line 907: std::stringstream output; : output << prefix->str() << prototype_->name() << : "{unit_type=\"" << MetricUnit::Name(prototype_->unit()) << "\"" << "}" : << " " << value() << "\n"; Using strings::Substitute() here instead would be more efficient, I guess. http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@917 PS6, Line 917: Prometheus doesn't support string gauges Then why to have this method at all? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@983 PS6, Line 983: std::stringstream output; : : output << prefix->str() << prototype_->name() << : "{unit_type=\"" << MetricUnit::Name(prototype_->unit()) << "\"" << "}" : << " " << value() << "\n"; : : output << prefix->str() << prototype_->name() << "_count" << : "{unit_type=\"" << MetricUnit::Name(prototype_->unit()) << "\"" << "}" : << " " << total_count() << "\n"; : : output << prefix->str() << prototype_->name() << "_sum" << : "{unit_type=\"" << MetricUnit::Name(prototype_->unit()) << "\"" << "}" : << " " << total_sum() << "\n"; : : writer->WriteEntry(&output); What sort of Prometheus metric does this map into? Is gauge allowed to have multiple lines? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1109 PS6, Line 1109: stringstream why not just ostringstream? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1111 PS6, Line 1111: "{unit_type=\"" << MetricUnit::Name(prototype_->unit()) << "\"" << ", " << "le=\"0.75\"} " : << histogram_->ValueAtPercentile(75) << "\n"; nit for here and below: the indent is messed up http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/metrics.cc@1130 PS6, Line 1130: output << prefix->str() << prototype_->name() << "_bucket" << : "{unit_type=\"" << MetricUnit::Name(prototype_->unit()) << "\"" << ", " << "le=\"+Inf\"} " : << histogram_->TotalCount() << "\n"; What if histogram_->TotalCount() value changes between this call and the call below when outputting 'x_count' value? http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h File src/kudu/util/prometheuswriter.h: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@17 PS6, Line 17: #ifndef KUDU_UTIL_PROMETHEUSWRITER_H : #define KUDU_UTIL_PROMETHEUSWRITER_H nit: use 'pragma' once instead http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@24 PS6, Line 24: private: : std::ostringstream* output_; nit: move this to be after the 'public' section http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.h@30 PS6, Line 30: const std::stringstream* Pass constant input parameters by constant reference: https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.cc File src/kudu/util/prometheuswriter.cc: http://gerrit.cloudera.org:8080/#/c/19133/6/src/kudu/util/prometheuswriter.cc@22 PS6, Line 22: void PrometheusWriter::WriteEntry(const std::stringstream* data) { : *output_ << data->str(); : } If the output data is converted into std::string anyways before writing into the output stream, why not to pass the parameter as std::string then? -- To view, visit http://gerrit.cloudera.org:8080/19133 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b2d01bd56f6f0f1b6f31cbce15e671d16521739 Gerrit-Change-Number: 19133 Gerrit-PatchSet: 6 Gerrit-Owner: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 14 Oct 2022 01:56:11 +0000 Gerrit-HasComments: Yes
