Repository: pinpoint-apm/pinpoint
Issue: Separate id used for internal trace map from transaction id used for context propagation
Labels: enhancement

A single id was used for both `transactionId` for context propagation, and trace map cache (as key).
This approach however does not work well for cases when a transaction is continued. For a continued transaction, using the `transactionId` as the map key may conflict with `transactionId` issued for new transactions.

Separating id used for trace map key (at least for continued transactions) would fix the problem.
This should be done using the least amount of atomic counters that covers id generation for 4 cases:
1. New Sampled Trace
2. New Unsampled Trace
3. Continued Sampled Trace
4. Continued Unsampled Trace


Issue comments:
Generate id differently (currently divides the `Long` space into 4)
Better to leave some space for future use.


Pull request review comments:

Patch that resolved the issue:
diff --git a/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/DefaultTransactionCounter.java b/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/DefaultTransactionCounter.java
index 6a68c5b172a6..004ae45d6580 100644
--- a/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/DefaultTransactionCounter.java
+++ b/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/DefaultTransactionCounter.java
@@ -29,7 +29,7 @@ public DefaultTransactionCounter(IdGenerator idGenerator) {
         }
         this.idGenerator = idGenerator;
     }
-
+    
     @Override
     public long getTransactionCount(SamplingType samplingType) {
         // overflow improbable
@@ -37,11 +37,11 @@ public long getTransactionCount(SamplingType samplingType) {
         case SAMPLED_NEW:
             return idGenerator.currentTransactionId() - IdGenerator.INITIAL_TRANSACTION_ID;
         case SAMPLED_CONTINUATION:
-            return Math.abs(idGenerator.currentContinuedTransactionId() - IdGenerator.INITIAL_CONTINUED_TRANSACTION_ID);
+            return Math.abs(idGenerator.currentContinuedTransactionId() - IdGenerator.INITIAL_CONTINUED_TRANSACTION_ID) / IdGenerator.DECREMENT_CYCLE;
         case UNSAMPLED_NEW:
-            return Math.abs(idGenerator.currentDisabledId() - IdGenerator.INITIAL_DISABLED_ID);
+            return Math.abs(idGenerator.currentDisabledId() - IdGenerator.INITIAL_DISABLED_ID) / IdGenerator.DECREMENT_CYCLE;
         case UNSAMPLED_CONTINUATION:
-            return idGenerator.currentContinuedDisabledId() - IdGenerator.INITIAL_CONTINUED_DISABLED_ID;
+            return Math.abs(idGenerator.currentContinuedDisabledId() - IdGenerator.INITIAL_CONTINUED_DISABLED_ID) / IdGenerator.DECREMENT_CYCLE;
         default:
             return 0L;
         }
diff --git a/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/IdGenerator.java b/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/IdGenerator.java
index 4daf8c250021..6fa0d2979082 100644
--- a/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/IdGenerator.java
+++ b/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/IdGenerator.java
@@ -24,14 +24,22 @@
  */
 public class IdGenerator {
 
-    // Positive value for sampled traces
-    public static final long INITIAL_TRANSACTION_ID = 1L;
-    public static final long INITIAL_CONTINUED_TRANSACTION_ID = Long.MAX_VALUE;
-    // Negative value for unsampled traces
-    public static final long INITIAL_DISABLED_ID = -1L;
-    public static final long INITIAL_CONTINUED_DISABLED_ID = Long.MIN_VALUE;
+    // TODO might be a good idea to refactor these into SamplingType
 
+    // Reserved negative space (0 ~ -1000)
     public static final long UNTRACKED_ID = 0L;
+    public static final long RESERVED_MAX = 0L;
+    public static final long RESERVED_MIN = -1000;
+
+    // Positive value for sampled new traces
+    public static final long INITIAL_TRANSACTION_ID = 1L;
+    // Negative value for sampled continuations, and unsampled new traces/continuations
+    public static final long INITIAL_CONTINUED_TRANSACTION_ID = RESERVED_MIN - 1; // -1001
+    public static final long INITIAL_DISABLED_ID = RESERVED_MIN - 2; // -1002
+    public static final long INITIAL_CONTINUED_DISABLED_ID = RESERVED_MIN - 3; // -1003
+
+    public static final int DECREMENT_CYCLE = 3;
+    public static final int NEGATIVE_DECREMENT_CYCLE = DECREMENT_CYCLE * -1;
 
     // Unique id for tracing a internal stacktrace and calculating a slow time of activethreadcount
     // moved here in order to make codes simpler for now
@@ -49,15 +57,15 @@ public long nextTransactionId() {
     }
 
     public long nextContinuedTransactionId() {
-        return this.continuedTransactionId.getAndDecrement();
+        return this.continuedTransactionId.getAndAdd(NEGATIVE_DECREMENT_CYCLE);
     }
 
     public long nextDisabledId() {
-        return this.disabledId.getAndDecrement();
+        return this.disabledId.getAndAdd(NEGATIVE_DECREMENT_CYCLE);
     }
 
     public long nextContinuedDisabledId() {
-        return this.continuedDisabledId.getAndIncrement();
+        return this.continuedDisabledId.getAndAdd(NEGATIVE_DECREMENT_CYCLE);
     }
 
     public long currentTransactionId() {


Test patch:
diff --git a/profiler/src/test/java/com/navercorp/pinpoint/profiler/context/DefaultTransactionCounterTest.java b/profiler/src/test/java/com/navercorp/pinpoint/profiler/context/DefaultTransactionCounterTest.java
new file mode 100644
index 000000000000..ddb7832e37d1
--- /dev/null
+++ b/profiler/src/test/java/com/navercorp/pinpoint/profiler/context/DefaultTransactionCounterTest.java
@@ -0,0 +1,210 @@
+/*
+ * Copyright 2015 NAVER Corp.
+ *
+ * Licensed 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 com.navercorp.pinpoint.profiler.context;
+
+import static org.junit.Assert.*;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import com.navercorp.pinpoint.profiler.context.TransactionCounter.SamplingType;
+
+/**
+ * @author HyunGil Jeong
+ */
+public class DefaultTransactionCounterTest {
+
+    private IdGenerator idGenerator;
+    private TransactionCounter transactionCounter;
+
+    @Before
+    public void setUp() throws Exception {
+        this.idGenerator = new IdGenerator();
+        this.transactionCounter = new DefaultTransactionCounter(this.idGenerator);
+    }
+
+    @Test
+    public void testNoTransaction_SAMPLED_NEW() {
+        // Given
+        final long expectedTransactionCount = 0L;
+        // When
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.SAMPLED_NEW);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testNoTransaction_SAMPLED_CONTINUATION() {
+        // Given
+        final long expectedTransactionCount = 0L;
+        // When
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.SAMPLED_CONTINUATION);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testNoTransaction_UNSAMPLED_NEW() {
+        // Given
+        final long expectedTransactionCount = 0L;
+        // When
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.UNSAMPLED_NEW);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testNoTransaction_UNSAMPLED_CONTINUATION() {
+        // Given
+        final long expectedTransactionCount = 0L;
+        // When
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.UNSAMPLED_CONTINUATION);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testSingleTransaction_SAMPLED_NEW() {
+        // Given
+        final long expectedTransactionCount = 1L;
+        // When
+        this.idGenerator.nextTransactionId();
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.SAMPLED_NEW);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testSingleTransaction_SAMPLED_CONTINUATION() {
+        // Given
+        final long expectedTransactionCount = 1L;
+        // When
+        this.idGenerator.nextContinuedTransactionId();
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.SAMPLED_CONTINUATION);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testSingleTransaction_UNSAMPLED_NEW() {
+        // Given
+        final long expectedTransactionCount = 1L;
+        // When
+        this.idGenerator.nextDisabledId();
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.UNSAMPLED_NEW);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testSingleTransaction_UNSAMPLED_CONTINUATION() {
+        // Given
+        final long expectedTransactionCount = 1L;
+        // When
+        this.idGenerator.nextContinuedDisabledId();
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.UNSAMPLED_CONTINUATION);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testMultipleTransaction_SAMPLED_NEW() {
+        // Given
+        final long expectedTransactionCount = 99L;
+        // When
+        for (int i = 0; i < expectedTransactionCount; ++i) {
+            this.idGenerator.nextTransactionId();
+        }
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.SAMPLED_NEW);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testMultipleTransaction_SAMPLED_CONTINUATION() {
+        // Given
+        final long expectedTransactionCount = 99L;
+        // When
+        for (int i = 0; i < expectedTransactionCount; ++i) {
+            this.idGenerator.nextContinuedTransactionId();
+        }
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.SAMPLED_CONTINUATION);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testMultipleTransaction_UNSAMPLED_NEW() {
+        // Given
+        final long expectedTransactionCount = 99L;
+        // When
+        for (int i = 0; i < expectedTransactionCount; ++i) {
+            this.idGenerator.nextDisabledId();
+        }
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.UNSAMPLED_NEW);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testMultipleTransaction_UNSAMPLED_CONTINUATION() {
+        // Given
+        final long expectedTransactionCount = 99L;
+        // When
+        for (int i = 0; i < expectedTransactionCount; ++i) {
+            this.idGenerator.nextContinuedDisabledId();
+        }
+        final long actualCount = this.transactionCounter.getTransactionCount(SamplingType.UNSAMPLED_CONTINUATION);
+        // Then
+        assertEquals(expectedTransactionCount, actualCount);
+    }
+
+    @Test
+    public void testTotalTransaction() {
+        // Given
+        final long expectedSampledNewCount = 19L;
+        final long expectedSampledContinuationCount = 29L;
+        final long expectedUnsampledNewCount = 0L;
+        final long expectedUnsampledContinuationCount = 9L;
+        final long expectedTotalCount = expectedSampledNewCount + expectedSampledContinuationCount + expectedUnsampledNewCount + expectedUnsampledContinuationCount;
+        // When
+        for (int i = 0; i < expectedSampledNewCount; ++i) {
+            this.idGenerator.nextTransactionId();
+        }
+        for (int i = 0; i < expectedSampledContinuationCount; ++i) {
+            this.idGenerator.nextContinuedTransactionId();
+        }
+        for (int i = 0; i < expectedUnsampledNewCount; ++i) {
+            this.idGenerator.nextDisabledId();
+        }
+        for (int i = 0; i < expectedUnsampledContinuationCount; ++i) {
+            this.idGenerator.nextContinuedDisabledId();
+        }
+        final long actualSampledNewCount = this.transactionCounter.getTransactionCount(SamplingType.SAMPLED_NEW);
+        final long actualSampledContinuationCount = this.transactionCounter.getTransactionCount(SamplingType.SAMPLED_CONTINUATION);
+        final long actualUnsampledNewCount = this.transactionCounter.getTransactionCount(SamplingType.UNSAMPLED_NEW);
+        final long actualUnsampledContinuationCount = this.transactionCounter.getTransactionCount(SamplingType.UNSAMPLED_CONTINUATION);
+        final long actualTotalCount = this.transactionCounter.getTotalTransactionCount();
+        // Then
+        assertEquals(expectedSampledNewCount, actualSampledNewCount);
+        assertEquals(expectedSampledContinuationCount, actualSampledContinuationCount);
+        assertEquals(expectedUnsampledNewCount, actualUnsampledNewCount);
+        assertEquals(expectedUnsampledContinuationCount, actualUnsampledContinuationCount);
+        assertEquals(expectedTotalCount, actualTotalCount);
+    }
+
+}