[#noissue] Move dataSender to MetaDataService

agent-module/profiler/src/main/java/com/navercorp/pinpoint/profiler/context/provider/metadata/SqlMetadataServiceProvider.java

@@ -57,12 +57,12 @@ public SqlMetaDataService get() {
 
         if (monitorConfig.isSqlStatEnable()) {
             SimpleCache<String, byte[]> sqlCache = simpleCacheFactory.newSqlUidCache();
-            SqlCacheService<byte[]> sqlCacheService = new SqlCacheService<>(dataSender, sqlCache, maxSqlLength);
-            return new SqlUidMetaDataService(sqlCacheService);
+            SqlCacheService<byte[]> sqlCacheService = new SqlCacheService<>(sqlCache, maxSqlLength);
+            return new SqlUidMetaDataService(dataSender, sqlCacheService);
         } else {
             SimpleCache<String, Integer> sqlCache = simpleCacheFactory.newSqlCache();
-            SqlCacheService<Integer> sqlCacheService = new SqlCacheService<>(dataSender, sqlCache, maxSqlLength);
-            return new DefaultSqlMetaDataService(sqlCacheService);
+            SqlCacheService<Integer> sqlCacheService = new SqlCacheService<>(sqlCache, maxSqlLength);
+            return new DefaultSqlMetaDataService(dataSender, sqlCacheService);
         }
     }
 }

agent-module/profiler/src/main/java/com/navercorp/pinpoint/profiler/metadata/DefaultSqlMetaDataService.java

@@ -17,7 +17,7 @@
 package com.navercorp.pinpoint.profiler.metadata;
 
 import com.navercorp.pinpoint.bootstrap.context.ParsingResult;
-import com.navercorp.pinpoint.common.annotations.VisibleForTesting;
+import com.navercorp.pinpoint.common.profiler.message.DataConsumer;
 import com.navercorp.pinpoint.common.trace.AnnotationKey;
 import com.navercorp.pinpoint.common.util.IntStringStringValue;
 import com.navercorp.pinpoint.common.util.StringUtils;
@@ -35,8 +35,11 @@ public class DefaultSqlMetaDataService implements SqlMetaDataService {
 
     private final SqlCacheService<Integer> sqlCacheService;
 
-    public DefaultSqlMetaDataService(SqlCacheService<Integer> sqlCacheService) {
+    private final DataConsumer<MetaDataType> dataSender;
+
+    public DefaultSqlMetaDataService(DataConsumer<MetaDataType> dataSender, SqlCacheService<Integer> sqlCacheService) {
         this.sqlCacheService = Objects.requireNonNull(sqlCacheService, "sqlCacheService");
+        this.dataSender = Objects.requireNonNull(dataSender, "dataSender");
     }
 
     @Override
@@ -53,10 +56,14 @@ public Annotation<?> newSqlAnnotation(ParsingResult parsingResult, String bindVa
             throw new IllegalStateException("Unexpected DefaultParsingResult :" + parsingResult);
         }
 
-
         final ParsingResultInternal<Integer> result = (DefaultParsingResult) parsingResult;
         if (result != EMPTY_RESULT) {
-            this.sqlCacheService.cacheSql(result, DefaultSqlMetaDataService::newSqlMetaData);
+            boolean isNewValue = this.sqlCacheService.cacheSql(result);
+
+            if (isNewValue) {
+                final MetaDataType sqlMetaData = new SqlMetaData(result.getId(), result.getSql());
+                dataSender.send(sqlMetaData);
+            }
         }
 
         String output = StringUtils.defaultIfEmpty(parsingResult.getOutput(), null);
@@ -65,9 +72,4 @@ public Annotation<?> newSqlAnnotation(ParsingResult parsingResult, String bindVa
         final IntStringStringValue sqlValue = new IntStringStringValue(result.getId(), output, bindValue);
         return Annotations.of(AnnotationKey.SQL_ID.getCode(), sqlValue);
     }
-
-    @VisibleForTesting
-    static MetaDataType newSqlMetaData(ParsingResultInternal<Integer> parsingResult) {
-        return new SqlMetaData(parsingResult.getId(), parsingResult.getSql());
-    }
 }

agent-module/profiler/src/main/java/com/navercorp/pinpoint/profiler/metadata/SqlCacheService.java

@@ -1,32 +1,25 @@
 package com.navercorp.pinpoint.profiler.metadata;
 
-import com.navercorp.pinpoint.common.profiler.message.DataConsumer;
 import com.navercorp.pinpoint.common.util.StringUtils;
 import com.navercorp.pinpoint.profiler.cache.Cache;
 import com.navercorp.pinpoint.profiler.cache.Result;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
-import java.util.Objects;
-import java.util.function.Function;
-
 public class SqlCacheService<ID> {
     protected final Logger logger = LogManager.getLogger(this.getClass());
     protected final boolean isDebug = logger.isDebugEnabled();
 
     private final CachingSqlNormalizer<ParsingResultInternal<ID>> cachingSqlNormalizer;
 
-    private final DataConsumer<MetaDataType> dataSender;
-
     private final int trimSqlLength;
 
-    public SqlCacheService(DataConsumer<MetaDataType> dataSender, Cache<String, Result<ID>> sqlCache, int trimSqlLength) {
-        this.dataSender = Objects.requireNonNull(dataSender, "dataSender");
+    public SqlCacheService(Cache<String, Result<ID>> sqlCache, int trimSqlLength) {
         this.cachingSqlNormalizer = new DefaultCachingSqlNormalizer<>(sqlCache);
         this.trimSqlLength = trimSqlLength;
     }
 
-    public boolean cacheSql(ParsingResultInternal<ID> parsingResult, Function<ParsingResultInternal<ID>, MetaDataType> converter) {
+    public boolean cacheSql(ParsingResultInternal<ID> parsingResult) {
         if (parsingResult == null) {
             return false;
         }
@@ -43,12 +36,6 @@ public boolean cacheSql(ParsingResultInternal<ID> parsingResult, Function<Parsin
                 String trimmedSql = StringUtils.abbreviate(parsingResult.getSql(), trimSqlLength);
                 parsingResult.setSql(trimmedSql);
             }
-
-            // isNewValue means that the value is newly cached.
-            // So the sql could be new one. We have to send sql metadata to collector.
-            final MetaDataType sqlMetaData = converter.apply(parsingResult);
-
-            this.dataSender.send(sqlMetaData);
         } else {
             if (isDebug) {
                 logger.debug("cache hit {}", parsingResult);

agent-module/profiler/src/main/java/com/navercorp/pinpoint/profiler/metadata/SqlUidMetaDataService.java

@@ -1,7 +1,7 @@
 package com.navercorp.pinpoint.profiler.metadata;
 
 import com.navercorp.pinpoint.bootstrap.context.ParsingResult;
-import com.navercorp.pinpoint.common.annotations.VisibleForTesting;
+import com.navercorp.pinpoint.common.profiler.message.DataConsumer;
 import com.navercorp.pinpoint.common.trace.AnnotationKey;
 import com.navercorp.pinpoint.common.util.BytesStringStringValue;
 import com.navercorp.pinpoint.common.util.StringUtils;
@@ -16,8 +16,11 @@ public class SqlUidMetaDataService implements SqlMetaDataService {
 
     private final SqlCacheService<byte[]> sqlCacheService;
 
-    public SqlUidMetaDataService(SqlCacheService<byte[]> sqlCacheService) {
+    private final DataConsumer<MetaDataType> dataSender;
+
+    public SqlUidMetaDataService(DataConsumer<MetaDataType> dataSender, SqlCacheService<byte[]> sqlCacheService) {
         this.sqlCacheService = Objects.requireNonNull(sqlCacheService, "sqlCacheService");
+        this.dataSender = Objects.requireNonNull(dataSender, "dataSender");
     }
 
     @Override
@@ -36,7 +39,12 @@ public Annotation<?> newSqlAnnotation(ParsingResult parsingResult, String bindVa
 
         final ParsingResultInternal<byte[]> result = (UidParsingResult) parsingResult;
         if (result != EMPTY_RESULT) {
-            this.sqlCacheService.cacheSql(result, SqlUidMetaDataService::newSqlUidMetaData);
+            boolean isNewValue = this.sqlCacheService.cacheSql(result);
+
+            if (isNewValue) {
+                final MetaDataType sqlMetaData = new SqlUidMetaData(result.getId(), result.getSql());
+                dataSender.send(sqlMetaData);
+            }
         }
 
         String output = StringUtils.defaultIfEmpty(parsingResult.getOutput(), null);
@@ -45,9 +53,4 @@ public Annotation<?> newSqlAnnotation(ParsingResult parsingResult, String bindVa
         final BytesStringStringValue sqlValue = new BytesStringStringValue(result.getId(), output, bindValue);
         return Annotations.of(AnnotationKey.SQL_UID.getCode(), sqlValue);
     }
-
-    @VisibleForTesting
-    static MetaDataType newSqlUidMetaData(ParsingResultInternal<byte[]> parsingResult) {
-        return new SqlUidMetaData(parsingResult.getId(), parsingResult.getSql());
-    }
 }

agent-module/profiler/src/test/java/com/navercorp/pinpoint/profiler/cache/UidCacheTest.java

@@ -0,0 +1,35 @@
+package com.navercorp.pinpoint.profiler.cache;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class UidCacheTest {
+    static int LENGTH_LIMIT = 100;
+
+    UidCache sut = new UidCache(100, new UidGenerator.Murmur(), LENGTH_LIMIT);
+
+    static String veryLongString() {
+        StringBuilder a = new StringBuilder();
+        for (int i = 0; i < LENGTH_LIMIT + 1; i++) {
+            a.append("a");
+        }
+        return a.toString();
+    }
+
+    @Test
+    void bypassCache() {
+        // given
+        String veryLongString = veryLongString();
+
+        // when
+        Result<byte[]> result1 = sut.put(veryLongString);
+        Result<byte[]> result2 = sut.put(veryLongString);
+
+        // then
+        assertArrayEquals(result1.getId(), result2.getId());
+        assertTrue(result1.isNewValue());
+        assertTrue(result2.isNewValue());
+    }
+}

agent-module/profiler/src/test/java/com/navercorp/pinpoint/profiler/metadata/SqlCacheServiceTest.java

@@ -16,68 +16,54 @@
 
 package com.navercorp.pinpoint.profiler.metadata;
 
-import com.navercorp.pinpoint.common.profiler.message.DataConsumer;
 import com.navercorp.pinpoint.profiler.cache.SimpleCache;
-import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import org.mockito.Mock;
-import org.mockito.junit.jupiter.MockitoExtension;
 
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 /**
  * @author Woonduk Kang(emeroad)
  */
-@ExtendWith(MockitoExtension.class)
 public class SqlCacheServiceTest {
-    private static final int MAX_LENGTH = 1000;
+    static final int MAX_LENGTH = 1000;
 
-    private SqlCacheService<Integer> sut;
+    SqlCacheService<Integer> sut;
 
-    @Mock
-    private DataConsumer<MetaDataType> dataSender;
+    static String veryLongString() {
+        StringBuilder builder = new StringBuilder();
+        for (int i = 0; i < MAX_LENGTH + 100; i++) {
+            builder.append("a");
+        }
+        return builder.toString();
+    }
 
     @BeforeEach
-    public void setUp() {
+    void setUp() {
         SimpleCache<String, Integer> sqlCache = SimpleCache.newIdCache(100);
-        sut = new SqlCacheService<>(dataSender, sqlCache, MAX_LENGTH);
+        sut = new SqlCacheService<>(sqlCache, MAX_LENGTH);
     }
 
     @Test
-    public void cacheSql() {
+    void cacheSql() {
         final String sql = "select * from A";
         final ParsingResultInternal<Integer> parsingResult = new DefaultParsingResult(sql);
 
-        boolean newValue = sut.cacheSql(parsingResult, DefaultSqlMetaDataService::newSqlMetaData);
-        boolean notNewValue = sut.cacheSql(parsingResult, DefaultSqlMetaDataService::newSqlMetaData);
+        boolean newValue = sut.cacheSql(parsingResult);
+        boolean notNewValue = sut.cacheSql(parsingResult);
 
         assertTrue(newValue);
-        verify(dataSender).send(any(SqlMetaData.class));
-
-        Assertions.assertFalse(notNewValue);
-        verifyNoMoreInteractions(dataSender);
+        assertFalse(notNewValue);
     }
 
     @Test
-    public void trimSql() {
+    void trimSql() {
         final String sql = veryLongString();
         final ParsingResultInternal<Integer> parsingResult = new DefaultParsingResult(sql);
 
-        sut.cacheSql(parsingResult, DefaultSqlMetaDataService::newSqlMetaData);
+        sut.cacheSql(parsingResult);
 
         assertTrue(parsingResult.getSql().length() < sql.length());
     }
-
-    private String veryLongString() {
-        StringBuilder builder = new StringBuilder();
-        for (int i = 0; i < MAX_LENGTH + 100; i++) {
-            builder.append("a");
-        }
-        return builder.toString();
-    }
 }

agent-module/profiler/src/test/java/com/navercorp/pinpoint/profiler/metadata/SqlUidMetaDataServiceTest.java

@@ -13,37 +13,33 @@
 
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
 @ExtendWith(MockitoExtension.class)
 class SqlUidMetaDataServiceTest {
-    static int LENGTH_LIMIT = 100;
-
     SqlUidMetaDataService sut;
-    SqlCacheService<byte[]> sqlCacheService;
 
     @Mock
     DataConsumer<MetaDataType> dataSender;
 
     @BeforeEach
     void setUp() {
-        UidCache sqlCache = new UidCache(100, new UidGenerator.Murmur(), LENGTH_LIMIT);
-        sqlCacheService = new SqlCacheService<>(dataSender, sqlCache, 1000);
-        sut = new SqlUidMetaDataService(sqlCacheService);
+        UidCache sqlCache = new UidCache(100, new UidGenerator.Murmur(), 1000);
+        SqlCacheService<byte[]> sqlCacheService = new SqlCacheService<>(sqlCache, 1000);
+        sut = new SqlUidMetaDataService(dataSender, sqlCacheService);
     }
 
     @Test
     void sameSqlSameId() {
         String sql = "select * from A";
 
         UidParsingResult parsingResult1 = (UidParsingResult) sut.wrapSqlResult(sql);
-        assertNew(parsingResult1);
+        sut.newSqlAnnotation(parsingResult1, null);
 
         UidParsingResult parsingResult2 = (UidParsingResult) sut.wrapSqlResult(sql);
-        assertCached(parsingResult2);
+        sut.newSqlAnnotation(parsingResult2, null);
 
         assertSameId(parsingResult1, parsingResult2);
         verify(dataSender, times(1)).send(any(SqlUidMetaData.class));
@@ -54,12 +50,12 @@ void sameSqlSameId_clearCache() {
         String sql = "select * from A";
 
         UidParsingResult parsingResult1 = (UidParsingResult) sut.wrapSqlResult(sql);
-        assertNew(parsingResult1);
+        sut.newSqlAnnotation(parsingResult1, null);
 
         setUp();
 
         UidParsingResult parsingResult2 = (UidParsingResult) sut.wrapSqlResult(sql);
-        assertNew(parsingResult2);
+        sut.newSqlAnnotation(parsingResult2, null);
 
         assertSameId(parsingResult1, parsingResult2);
         verify(dataSender, times(2)).send(any(SqlUidMetaData.class));
@@ -69,45 +65,16 @@ void sameSqlSameId_clearCache() {
     void differentSqlDifferentId() {
         String sql1 = "select * from A";
         UidParsingResult parsingResult1 = (UidParsingResult) sut.wrapSqlResult(sql1);
-        assertNew(parsingResult1);
+        sut.newSqlAnnotation(parsingResult1, null);
 
         String sql2 = "select * from B";
         UidParsingResult parsingResult2 = (UidParsingResult) sut.wrapSqlResult(sql2);
-        assertNew(parsingResult2);
+        sut.newSqlAnnotation(parsingResult2, null);
 
         assertDifferentId(parsingResult1, parsingResult2);
         verify(dataSender, times(2)).send(any(SqlUidMetaData.class));
     }
 
-    @Test
-    void bypassCache() {
-        String veryLongSql = veryLongSql();
-
-        UidParsingResult parsingResult1 = (UidParsingResult) sut.wrapSqlResult(veryLongSql);
-        assertNew(parsingResult1);
-
-        UidParsingResult parsingResult2 = (UidParsingResult) sut.wrapSqlResult(veryLongSql);
-        assertNew(parsingResult2);
-
-        assertSameId(parsingResult1, parsingResult2);
-    }
-
-    String veryLongSql() {
-        StringBuilder a = new StringBuilder();
-        for (int i = 0; i < LENGTH_LIMIT + 1; i++) {
-            a.append("a");
-        }
-        return a.toString();
-    }
-
-    void assertNew(UidParsingResult parsingResult) {
-        assertTrue(sqlCacheService.cacheSql(parsingResult, SqlUidMetaDataService::newSqlUidMetaData));
-    }
-
-    void assertCached(UidParsingResult parsingResult) {
-        assertFalse(sqlCacheService.cacheSql(parsingResult, SqlUidMetaDataService::newSqlUidMetaData));
-    }
-
     static void assertSameId(UidParsingResult parsingResult1, UidParsingResult parsingResult2) {
         assertArrayEquals(parsingResult1.getId(), parsingResult2.getId());
     }