ctubbsii commented on code in PR #4218: URL: https://github.com/apache/accumulo/pull/4218#discussion_r1481859448
########## test/src/main/java/org/apache/accumulo/test/metrics/TestLoggingRegistryFactory.java: ########## @@ -0,0 +1,43 @@ +/* + * 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 + * + * https://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.accumulo.test.metrics; + +import org.apache.accumulo.core.metrics.MeterRegistryFactory; Review Comment: I just noticed that the factory is in not in a public API package, or an SPI package. It should probably be in core/spi/metrics, not just core/metrics. If users are expected to implement this SPI class, MeterRegistryFactory should be in the SPI package. ########## test/src/main/java/org/apache/accumulo/test/metrics/TestLoggingRegistryFactory.java: ########## @@ -0,0 +1,43 @@ +/* + * 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 + * + * https://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.accumulo.test.metrics; + +import org.apache.accumulo.core.metrics.MeterRegistryFactory; + +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.logging.LoggingMeterRegistry; +import io.micrometer.core.instrument.logging.LoggingRegistryConfig; + +/** + * Set the following properties to use this MeterRegistryFactory + * + * <p> + * Property.GENERAL_MICROMETER_ENABLED = true + * <p> + * Property.GENERAL_MICROMETER_FACTORY = TestLoggingRegistryFactory.class.getName() + */ +public class TestLoggingRegistryFactory implements MeterRegistryFactory { Review Comment: I think this is a good basic implementation for a reference implementation. This class name could be set as the default value for the factory property, and this class could also live in the core/spi/metrics package. I'd suggest renaming it to "BasicLoggingMeterRegistryFactory" ########## test/src/main/java/org/apache/accumulo/test/metrics/LoggingMetricsIT.java: ########## @@ -0,0 +1,74 @@ +/* + * 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 + * + * https://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.accumulo.test.metrics; + +import java.time.Duration; + +import org.apache.accumulo.core.conf.Property; +import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl; +import org.apache.hadoop.conf.Configuration; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +public class LoggingMetricsIT extends MetricsIT { + + @Override + protected Duration defaultTimeout() { + return Duration.ofMinutes(4); + } + + @Override + protected void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) { + cfg.setNumTservers(2); + cfg.setProperty(Property.GC_CYCLE_START, "1s"); + cfg.setProperty(Property.GC_CYCLE_DELAY, "1s"); + cfg.setProperty(Property.MANAGER_FATE_METRICS_MIN_UPDATE_INTERVAL, "1s"); + + // Tell the server processes to use a StatsDMeterRegistry that will be configured Review Comment: This incorrectly says StatsD when its a Logging impl. ########## test/src/main/java/org/apache/accumulo/test/metrics/LoggingMetricsIT.java: ########## @@ -0,0 +1,74 @@ +/* + * 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 + * + * https://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.accumulo.test.metrics; + +import java.time.Duration; + +import org.apache.accumulo.core.conf.Property; +import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl; +import org.apache.hadoop.conf.Configuration; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +public class LoggingMetricsIT extends MetricsIT { + + @Override + protected Duration defaultTimeout() { + return Duration.ofMinutes(4); + } + + @Override + protected void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) { + cfg.setNumTservers(2); + cfg.setProperty(Property.GC_CYCLE_START, "1s"); + cfg.setProperty(Property.GC_CYCLE_DELAY, "1s"); + cfg.setProperty(Property.MANAGER_FATE_METRICS_MIN_UPDATE_INTERVAL, "1s"); + + // Tell the server processes to use a StatsDMeterRegistry that will be configured + // to push all metrics to the sink we started. + cfg.setProperty(Property.GENERAL_MICROMETER_ENABLED, "true"); + cfg.setProperty(Property.GENERAL_MICROMETER_FACTORY, + TestLoggingRegistryFactory.class.getName()); + } + + @Test + @Disabled + @Override + public void confirmMetricsPublished() throws Exception { + // Override does nothing + } + + @Test + @Disabled + @Override + public void metricTags() throws Exception { + // Override does nothing + } + + @Test + public void doWorkToGenerateLogs() throws Exception { + // Metrics are pushed every 1m using a logger with the name LoggingMeterRegistry + // we need the test to take longer than 1m + for (int i = 0; i < 10; i++) { + doWorkToGenerateMetrics(); + Thread.sleep(10_000); Review Comment: How are the logs verified? ########## test/src/main/resources/log4j2-test.properties: ########## @@ -152,29 +152,6 @@ logger.38.level = debug logger.39.name = org.apache.accumulo.manager.Manager logger.39.level = trace -property.metricsFilename = ./target/test-metrics - -# appender.metrics.type = Console -appender.metrics.type = RollingFile -appender.metrics.name = LoggingMetricsOutput -appender.metrics.fileName = ${metricsFilename}.metrics -appender.metrics.filePattern = ${metricsFilename}-%d{MM-dd-yy-HH}-%i.metrics.gz -appender.metrics.layout.type = PatternLayout -appender.metrics.layout.pattern = METRICS: %d{ISO8601}, %m%n -appender.metrics.policies.type = Policies -appender.metrics.policies.time.type = TimeBasedTriggeringPolicy -appender.metrics.policies.time.interval = 1 -appender.metrics.policies.time.modulate = true -appender.metrics.policies.size.type = SizeBasedTriggeringPolicy -appender.metrics.policies.size.size=100MB -appender.metrics.strategy.type = DefaultRolloverStrategy -appender.metrics.strategy.max = 10 - -logger.metrics.name = org.apache.accumulo.metrics -logger.metrics.level = info -logger.metrics.additivity = false -logger.metrics.appenderRef.metrics.ref = LoggingMetricsOutput - Review Comment: :+1: I love seeing this stuff go away from our config, even though it's just test config. I'm not really sure how it was previously being used, though. -- 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]
