dsmiley commented on code in PR #3665:
URL: https://github.com/apache/solr/pull/3665#discussion_r2364495223
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudAuthTestCase.java:
##########
@@ -336,46 +336,6 @@ private static Map<String, DataPointSnapshot>
getMetricValues(
return metrics;
}
- /**
- * Common test method to be able to check audit metrics
- *
- * @param className the class name to be used for composing prefix, e.g.
- * "SECURITY./auditlogging/SolrLogAuditLoggerPlugin"
- */
- protected void assertAuditMetricsMinimums(
- MiniSolrCloudCluster cluster, String className, int count, int errors)
Review Comment:
don't ever hold yourself back from making changes in test framework outside
of the usual suspects
##########
solr/core/src/java/org/apache/solr/metrics/SolrMetricManager.java:
##########
@@ -157,6 +161,23 @@ public class SolrMetricManager {
private final ConcurrentMap<String, MeterProviderAndReaders>
meterProviderAndReaders =
new ConcurrentHashMap<>();
+ private static final List<Double> SOLR_NANOSECOND_HISTOGRAM_BOUNDARIES =
+ Arrays.asList(
Review Comment:
`List.of` almost always instead of that.
##########
solr/core/src/test/org/apache/solr/security/AuditLoggerIntegrationTest.java:
##########
@@ -102,18 +110,50 @@ public void tearDown() throws Exception {
public void testSynchronous() throws Exception {
setupCluster(false, null, false);
runThreeTestAdminCommands();
+
+ Thread.sleep(100);
Review Comment:
Please don't add sleeps to tests; they are evil. They are brittle and
en-masse they slow things down. See `RetryUtil` for a possible alternative
approach. At least comment on why the thing we want to assert isn't
necessarily observable immediately..
##########
solr/core/src/java/org/apache/solr/security/AuditLoggerPlugin.java:
##########
@@ -261,39 +260,70 @@ public void setFormatter(AuditEventFormatter formatter) {
this.formatter = formatter;
}
- // TODO SOLR-17458: Migrate to Otel
@Override
public void initializeMetrics(
SolrMetricsContext parentContext, Attributes attributes, final String
scope) {
solrMetricsContext = parentContext.getChildContext(this);
String className = this.getClass().getSimpleName();
log.debug("Initializing metrics for {}", className);
- numErrors = solrMetricsContext.meter("errors", getCategory().toString(),
scope, className);
- numLost = solrMetricsContext.meter("lost", getCategory().toString(),
scope, className);
- numLogged = solrMetricsContext.meter("count", getCategory().toString(),
scope, className);
+
+ Attributes attrsWithCategory =
+ Attributes.builder()
+ .putAll(attributes)
+ .put(CATEGORY_ATTR, Category.SECURITY.toString())
Review Comment:
@mlbiscoc is there any point to the category attribute in metrics? It seems
more descriptional in nature, and not really something anyone would filter on.
##########
solr/core/src/test/org/apache/solr/security/AuditLoggerIntegrationTest.java:
##########
@@ -102,18 +110,50 @@ public void tearDown() throws Exception {
public void testSynchronous() throws Exception {
setupCluster(false, null, false);
runThreeTestAdminCommands();
+
+ Thread.sleep(100);
+
assertThreeTestAdminEvents();
- assertAuditMetricsMinimums(
- testHarness.get().cluster,
CallbackAuditLoggerPlugin.class.getSimpleName(), 3, 0);
+ assertAuditMetricsMinimums(3, 0);
+ Labels labels = getDefaultAuditLoggerMetricsLabelsBuilder().build();
+
+ assertGaugeMetricValue("solr_auditlogger_async_enabled", labels, 0);
+
+ HistogramSnapshot.HistogramDataPointSnapshot snapshot =
Review Comment:
nit: I love "var" for such cases
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
