Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count sub-entities when merge metrics ...................................................................... Patch Set 2: (8 comments) Didn't make it all the way through; wanted to see if we could avoid the dynamic metric prototype usage. http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h@711 PS2, Line 711: class MergedEntityPrototypes { Needs docs. http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h@713 PS2, Line 713: static MergedEntityPrototypes* GetInstance() { Nit: indented too much. http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@358 PS2, Line 358: entity_id = attrs[merge_rule->attribute_to_merge_by]; What happens if this map lookup fails? Seems like we'll set entity_id to an empty string; do we instead want to return an error? http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@375 PS2, Line 375: exist existing http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@377 PS2, Line 377: FindPtrOrNull(entity_collection, entries_count_prototype).get()); This should be a FindOrDie variant if there's an expectation that the result always exists (and the code currently assumes that). http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@461 PS2, Line 461: MergedEntityPrototypes::~MergedEntityPrototypes() { : for (const auto& prototype : prototypes_) { : delete[] prototype.second.get()->entity_type(); : } : } An AutoReleasePool might be a more natural choice. http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@467 PS2, Line 467: std:: Drop the prefix. http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@479 PS2, Line 479: MetricPrototype::CtorArgs(entity_type_tmp, This effectively adds "dynamic" metric prototype definition to the metrics library. Note that all prototypes are statically defined today (there's a comment near the top of metrics.h about this), and that has advantages: apart from the odd lifecycle semantics of dynamic prototypes, it also means it's really easy to generate a full schema up front using the prototypes and send that schema to monitoring tools. In this particular case, "entity_type_tmp" will always be one of several entity types (i.e. "server", "tablet", and "table"), right? So perhaps we could statically define it too, by adjusting METRIC_DEFINE_entity() to also define a gauge prototype for the merged entity count? -- To view, visit http://gerrit.cloudera.org:8080/14128 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1 Gerrit-Change-Number: 14128 Gerrit-PatchSet: 2 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 26 Aug 2019 22:44:46 +0000 Gerrit-HasComments: Yes
