suddendust commented on code in PR #14155: URL: https://github.com/apache/pinot/pull/14155#discussion_r1812266792
########## pinot-common/src/test/java/org/apache/pinot/common/metrics/prometheus/ServerPrometheusMetricsTest.java: ########## @@ -0,0 +1,196 @@ +/** + * 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. + */ +package org.apache.pinot.common.metrics.prometheus; + +import java.util.List; +import java.util.concurrent.TimeUnit; +import org.apache.pinot.common.metrics.ServerGauge; +import org.apache.pinot.common.metrics.ServerMeter; +import org.apache.pinot.common.metrics.ServerMetrics; +import org.apache.pinot.common.metrics.ServerTimer; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + + +public abstract class ServerPrometheusMetricsTest extends PinotPrometheusMetricsTest { + //all exported server metrics have this prefix + private static final String EXPORTED_METRIC_PREFIX = "pinot_server_"; + + private static final List<ServerMeter> METERS_ACCEPTING_CLIENT_ID = Review Comment: Any new metric that'll be added will have `tableNameWithType` supplied to them. Here's the relevant piece of code for `ServerMeter`: ``` } else { if (METERS_ACCEPTING_CLIENT_ID.contains(serverMeter)) { addMeterWithLabels(serverMeter, CLIENT_ID); assertMeterExportedCorrectly(serverMeter.getMeterName(), ExportedLabels.PARTITION_TABLENAME_TABLETYPE_KAFKATOPIC); } else if (METERS_ACCEPTING_RAW_TABLE_NAMES.contains(serverMeter)) { addMeterWithLabels(serverMeter, ExportedLabelValues.TABLENAME); assertMeterExportedCorrectly(serverMeter.getMeterName(), ExportedLabels.TABLENAME); } else { //we pass tableNameWithType to all remaining meters addMeterWithLabels(serverMeter, TABLE_NAME_WITH_TYPE); assertMeterExportedCorrectly(serverMeter.getMeterName(), ExportedLabels.TABLENAME_TABLETYPE); } ``` We explicitly have to create a list for `METERS_ACCEPTING_CLIENT_ID` or `METERS_ACCEPTING_RAW_TABLE_NAMES` et. al. because this is how these metrics are being used right now in code. Ideally, we should not be using `rawTableName` anywhere but `tableNameWithType`. However, to maintain backward compatibility with metrics as they are being used currently, I have to explicitly filter them. Now suppose a new metric is added in which the user expects the `partition` as one of the exported labels. If he does not add an explicit check for this new metric in the test, then it would go to the `else` block (with `ExportedLabels.TABLENAME_TABLETYPE`) and the test would PASS - This would ensure that at least the `table` and `tabletype` labels are present. We'll need to document that if you want some special label, you'll need to add a test case for it explicitly otherwise you'll only end up getting `table` and `tableType`. -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
