ppkarwasz commented on code in PR #3532:
URL: https://github.com/apache/logging-log4j2/pull/3532#discussion_r1995167509


##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/UuidUtil.java:
##########
@@ -140,4 +142,47 @@ public static UUID getTimeBasedUuid() {
         final long most = timeLow | timeMid | TYPE1 | timeHi;
         return new UUID(most, LEAST);
     }
+
+    /**
+     * Generates a Type 4 UUID based on the deterministic LogEvent hash.
+     * Meant for generating consistent, correlatable UUID values across 
multiple Appenders for the same LogEvent.
+     *
+     * @param logEvent
+     * @return universally unique identifiers (UUID)
+     */
+    public static UUID getLogEventBasedUuid(LogEvent logEvent) {
+        // TODO: better hashing algorithm - include other LogEvent fields?
+        long epochSecond = logEvent.getInstant().getEpochSecond();
+        // Enable 'log4j.configuration.usePreciseClock' system property 
otherwise will be truncated to millis
+        long nanoOfSecond = logEvent.getInstant().getNanoOfSecond();
+        // Thread IDs typically increment from 0 producing a narrow range
+        long threadId = logEvent.getThreadId();
+
+        // Increase entropy
+        long most = mix(epochSecond, nanoOfSecond, threadId);
+        long least = mix(threadId, ~nanoOfSecond, epochSecond);
+        // Set UUID v4 bits
+        most &= 0xFFFFFFFFFFFF0FFFL;
+        most |= 0x0000000000004000L;

Review Comment:
   This looks more like a [UUID Type 
8](https://www.rfc-editor.org/rfc/rfc9562.html#name-uuid-version-8) 
(vendor-specific or experimental), not a UUID Type 4 (purely random).



##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/UuidUtil.java:
##########
@@ -140,4 +142,47 @@ public static UUID getTimeBasedUuid() {
         final long most = timeLow | timeMid | TYPE1 | timeHi;
         return new UUID(most, LEAST);
     }
+
+    /**
+     * Generates a Type 4 UUID based on the deterministic LogEvent hash.
+     * Meant for generating consistent, correlatable UUID values across 
multiple Appenders for the same LogEvent.
+     *
+     * @param logEvent
+     * @return universally unique identifiers (UUID)
+     */
+    public static UUID getLogEventBasedUuid(LogEvent logEvent) {
+        // TODO: better hashing algorithm - include other LogEvent fields?
+        long epochSecond = logEvent.getInstant().getEpochSecond();
+        // Enable 'log4j.configuration.usePreciseClock' system property 
otherwise will be truncated to millis
+        long nanoOfSecond = logEvent.getInstant().getNanoOfSecond();
+        // Thread IDs typically increment from 0 producing a narrow range
+        long threadId = logEvent.getThreadId();
+
+        // Increase entropy
+        long most = mix(epochSecond, nanoOfSecond, threadId);
+        long least = mix(threadId, ~nanoOfSecond, epochSecond);
+        // Set UUID v4 bits
+        most &= 0xFFFFFFFFFFFF0FFFL;
+        most |= 0x0000000000004000L;
+        least &= 0x3FFFFFFFFFFFFFFFL;
+        least |= 0x8000000000000000L;
+
+        return new UUID(most, least);
+    }
+
+    private static long mix(long v1, long v2, long v3) {
+        // XOR with large primes
+        long hash = v1 * 0x9E3779B97F4A7C15L;
+        hash ^= (v2 * 0xC6BC279692B5C323L);
+        hash ^= (v3 * 0x3243F6A8885A308DL);
+        // Scramble
+        hash ^= (hash >>> 33);
+        hash *= 0xff51afd7ed558ccdL;
+        hash ^= (hash >>> 33);
+        hash *= 0xc4ceb9fe1a85ec53L;
+        hash ^= (hash >>> 33);
+        // Add salt
+        hash ^= SALT;
+        return hash;
+    }

Review Comment:
   Can you explain the purpose of this mangling? This could actually decrease 
the number of possible hashes, not increase it.



##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/UuidUtil.java:
##########
@@ -50,6 +51,7 @@ public final class UuidUtil {
     private static final int HUNDRED_NANOS_PER_MILLI = 10000;
 
     private static final long LEAST = initialize(NetUtils.getMacAddress());
+    private static final long SALT = new SecureRandom().nextLong();

Review Comment:
   What is the impact of this on startup time, e.g. in a VM without a physical 
source of entropy?
   
   I think that `Random` might be enough, since this is not used for 
cryptographic purposes.



-- 
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: notifications-unsubscr...@logging.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to