Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22314 )
Change subject: KUDU-3563 Output tablet-level metrics in Prometheus ...................................................................... Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/22314/3/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/22314/3/src/kudu/util/metrics.cc@446 PS3, Line 446: > But at this point there is no table-level metric, so it would not be used in > production. That's not true -- Kudu masters expose metric entities of the 'table' type, both in tests and production. Did you try running this code against the '/prometheus_metrics' HTTP endpoint of kudu-master where at least one table is present? > I agree that it would make sense to create a test like that but in my opinion > it is out of the scope, if the scope is just make tablet metrics visible in > prometheus. An extra test scenario I requested to add was to check against various bugs and other issues in the code, similar to what are present as of PS3 in this patch. Having such test could help to weed out bugs and catch future regressions. Probably, MasterTest.TestMasterContentTypeHeader is not the best venue to explore that, but I guess you could figure it out and find other ways to make sure the code is run against all the metric entities that are exposed by Kudu servers, both kudu-master and kudu-tserver. In my opinion, the newly added functionality should have test coverage, at least at very basic, 'smoke'-style level. This patch lacks it as of PS5, and knowing this patch has an issue with processing of metric entities exposed by kudu-master metrics as of PS3, I'm not going to give my +2 for such a patch. Thank you. -- To view, visit http://gerrit.cloudera.org:8080/22314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id033a1410966167d9bb24f6a44c584e73c17a4af Gerrit-Change-Number: 22314 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Ádám Bakai <[email protected]> Gerrit-Comment-Date: Thu, 16 Jan 2025 18:40:30 +0000 Gerrit-HasComments: Yes
