ctubbsii commented on code in PR #3124:
URL: https://github.com/apache/accumulo/pull/3124#discussion_r1102144327
##########
assemble/conf/log4j2-service.properties:
##########
@@ -84,6 +84,9 @@ logger.accumulo.level = debug
#logger.audit.additivity = false
#logger.audit.appenderRef.audit.ref = AuditLogFiles
+logger.cid.name = org.apache.accumulo.core.logging.CorrelationIdLogger
Review Comment:
Generically, "Correlation ID" is an ambiguous term, because it doesn't
explain what two or more things it is correlating.
Calling it "Correlation ID" on the ScannerBase methods is fine, because the
fact that it's on the Scanner API implies it's correlating scan information.
However, as a logger, we will need to ensure that log messages that use this
CorrelationIdLogger include some context, so we know it's logging scan
information. This could be a simple as (pseudo-code):
`Logger("CorrelationIdLogger").log("Scan <id> ...")`. The fact that it shows up
in the logs as part of the CorrelationIdLogger tells us that it's a log message
to correlate information about something. But, the fact that it says it's a
"scan" in the message tells us that the correlation ID that is being provided
is one associated with a scan.
Presumably, other components than scans might also be able to log to the
CorrelationIdLogger with their own context-specific IDs. If that's not the
intent and this is intended only for scans, then it would be better to rename
this logger to something that is scan-specific, and then we don't need to
include "scan" in the message to add that important contextual information.
##########
core/src/main/java/org/apache/accumulo/core/logging/CorrelationIdLogger.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.core.logging;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.event.Level;
+
+public class CorrelationIdLogger {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(CorrelationIdLogger.class);
+ private static final String FORMAT = "(%s) %s";
+
+ /**
+ * Utility method that will log the msg and args to the calling class'
logger (classLogger) if the
+ * classLogger argument is supplied. The classLogger argument would be null
in the case where we
+ * don't want to log the same thing twice.
+ *
+ * @param level slf4j log level
+ * @param classLogger calling class' slf4j logger
+ * @param correlationId meaningful data that applications can use to
correlate server side
+ * information
+ * @param msg log message
+ * @param args log message arguments
+ */
+ public static void log(Level level, Logger classLogger, String
correlationId, String msg,
+ Object... args) {
+ if (classLogger != null && classLogger.isEnabledForLevel(level)) {
+ classLogger.atLevel(level).log(msg, args);
+ }
+ if (LOG.isEnabledForLevel(level)) {
+ if (args == null) {
+ LOG.atLevel(level).log(String.format(FORMAT, correlationId, msg));
+ } else {
+ LOG.atLevel(level).log(String.format(FORMAT, correlationId, msg),
args);
+ }
+ }
Review Comment:
Having two log messages seems excessive. I think if the original logger had
the necessary information, you can correlate the logs without logging a second
time.
I'm also thinking that the layers of formatting / interpolation here make
this very confusing to understand/maintain.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ActiveScanImpl.java:
##########
@@ -55,6 +55,7 @@ public class ActiveScanImpl extends ActiveScan {
private Map<String,Map<String,String>> ssio;
private String user;
private Authorizations authorizations;
+ private String correlationId;
Review Comment:
Honestly, I think `scanId` is probably much more straight-forward than
`correlationId`. Or even "scanLabel".
##########
core/src/main/java/org/apache/accumulo/core/logging/CorrelationIdLogger.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.core.logging;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.event.Level;
Review Comment:
I'm still a bit uncomfortable more tightly coupling ourselves to more SLF4J
APIs, and have reservations about using Level here. Log4j's Level caused us a
lot of problems removing it from our code, and although SLF4J's Level is maybe
situated differently in its API, I can't help but anticipate the potential
future problems we could have by using more of the logging framework's APIs in
our code. I'd really prefer to avoid it. I also think that what I suggested
before about having a logging lifecycle class, like what Keith did elsewhere
would be better than adding this Logger class. I know Keith said that was out
of scope for this PR... but I'm not so sure. If having that avoids this new
class I think that's better than adding this class. If it needs to be done
first, so it's a discrete task, that's fine... but I do think it's better than
adding this class.
--
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]
