Add ExternalMappingSourceProcessor #271

matvey-mtn · 2022-03-01T17:14:45Z

No description provided.

barakm · 2022-03-02T10:04:53Z

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

+            conn.setConnectTimeout(5000);
+            conn.setReadTimeout(5000);
+
+            if (conn.getResponseCode() != 200) {


Suggested change

if (conn.getResponseCode() != 200) {

if (conn.getResponseCode() != HttpURLConnection.HTTP_OK) {

barakm · 2022-03-02T10:07:30Z

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

+            conn.setConnectTimeout(5000);
+            conn.setReadTimeout(5000);


These constants should be parameters of the processor, with reasonable defaults such that users will usually not to fill these in, but in case of a problematic remote server, they can set higher values.

barakm · 2022-03-02T10:09:23Z

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

+        this.mappingSourceUrl = new URL(mappingSourceUrl);
+    }
+
+    public Map<String, List<String>> getMappings() {


Suggested change

public Map<String, List<String>> getMappings() {

public Map<String, List<String>> loadMappings() {

using the get* method pattern implies that you are reading a local field, not performing a remote call

barakm · 2022-03-02T10:10:23Z

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

+
+    private Pair<String, List<String>> toKeyValuePair(String inputLine) {
+        String[] keyVal = inputLine.split("=");
+        String[] split = keyVal[1].trim().split(",");


This will fail on an IndexOutOfBounds if the input is faulty and contains no '='.
It is better to validate the values explicitly

barakm · 2022-03-02T12:39:17Z

sawmill-core/src/main/java/io/logz/sawmill/processors/ExternalMappingSourceProcessor.java

+    private void startScheduledExecutor() {
+        ScheduledExecutorService scheduledRulesUpdaterExecutor = Executors.newScheduledThreadPool(1,
+                new ThreadFactoryBuilder().setNameFormat("refresh-mapping-executor").setDaemon(true).build());
+        scheduledRulesUpdaterExecutor.scheduleAtFixedRate(this::refreshExternalMapping, mappingRefreshPeriodInSeconds,
+                mappingRefreshPeriodInSeconds, TimeUnit.SECONDS);
+    }


Every processor will have its own separate thread, which mostly does nothing. This seems wasteful.
I think this thread pool can be static, and lazy initialized.

barakm · 2022-03-02T12:40:40Z

sawmill-core/src/main/java/io/logz/sawmill/processors/ExternalMappingSourceProcessor.java

+    }
+
+    private void refreshExternalMapping() {
+        keyValueMappingsCache = externalMappingsClient.getMappings();


This is not thread-safe. You need to mark this variable as 'volatile', or alternatively use the AtomicReference class (which does the same thing internally)

uh, shame on me 😄

barakm · 2022-03-02T12:46:49Z

sawmill-core/src/test/java/io/logz/sawmill/processors/ExternalMappingSourceProcessorTest.java

+        List<String> targetField = doc.getField(TARGET_FIELD_NAME);
+        assertThat(targetField).isEmpty();
+    }
+


I am missing some additional tests here for bad file formats: Empty file, file without '=' character, file with empty key or empty value.
All of these things will happen sooner or later.

Also, there should be a test for non-200 responses.

added all test cases except

file with an empty key or empty value.

will add it tomorrow

…g_cr_fixes

Fix build

barakm · 2022-03-02T19:23:42Z

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

+            if (conn.getResponseCode() != HttpURLConnection.HTTP_OK) {
+                throw new HttpRequestExecutionException("Couldn't load external mappings. Message: " + conn.getResponseMessage());
+            }
+


Add a check for content-length, verifying that the downloaded file is overall < 50MB. Otherwise, a mega file could kill the JVM.

I would prefer to do it in a separate PR. Will open a ticket for it.

barakm · 2022-03-02T19:24:14Z

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

+
+            try (BufferedReader in = new BufferedReader(new InputStreamReader(conn.getInputStream()))) {
+                String inputLine;
+                while ((inputLine = in.readLine()) != null) {


Add a validation for number of lines. More than 50K lines should fail.

I would prefer to do it in a separate PR. Will open a ticket for it.

DanMelman

Looking good!
Added some comments.

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

sawmill-core/src/main/java/io/logz/sawmill/processors/ExternalMappingSourceProcessor.java

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

sawmill-core/src/main/java/io/logz/sawmill/processors/ExternalMappingSourceProcessor.java

* CR fixes - added tests for escape characters - added test with empty values

matvey-mtn · 2022-03-03T20:34:22Z

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

DanMelman · 2022-03-03T20:43:29Z

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

Missed that test. this is indeed enough coverage.

DanMelman · 2022-03-03T20:44:45Z

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

Which test is that? Still can't find it

matvey-mtn · 2022-03-04T12:12:38Z

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

Which test is that? Still can't find it

@DanMelman all tests that trigger processor.process(doc) will trigger refreshExternalMapping() on the first invocation.

DanMelman · 2022-03-04T12:14:41Z

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

Which test is that? Still can't find it

@DanMelman all tests that trigger processor.process(doc) will trigger refreshExternalMapping() on the first invocation.

@matvey-mtn My intention was to test the map update/replacement, including the case where a reload failed and we need to validate the old map stayed

matvey-mtn · 2022-03-04T12:16:01Z

Which test is that? Still can't find it

@DanMelman all tests that trigger processor.process(doc) will trigger refreshExternalMapping() on the first invocation.

@matvey-mtn My intention was to test the map update/replacement, including the case where a reload failed and we need to validate the old map stayed

@DanMelman ok, I will add something

…g_source_pipeline

DanMelman · 2022-03-06T11:16:25Z

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

+
+public class ExternalMappingsClient {
+
+    private final Logger logger = LoggerFactory.getLogger(ExternalMappingsClient.class);


DanMelman · 2022-03-06T11:17:36Z

sawmill-core/src/main/java/io/logz/sawmill/http/ExternalMappingsClient.java

+    private final int readTimeout;
+
+    public ExternalMappingsClient(ExternalMappingSourceProcessor.Configuration configuration) throws MalformedURLException {
+        this.mappingSourceUrl = new URL(requireNonNull(configuration.getMappingSourceUrl()));


I think requireNonNull is redundant, it will just be malformed url

DanMelman

👏

Add ExternalMappingSourceProcessor

09cb398

matvey-mtn requested review from barakm and DanMelman March 1, 2022 17:14

barakm reviewed Mar 2, 2022

View reviewed changes

matvey-mtn added 7 commits March 2, 2022 15:24

Add validations, tests, and minimumRefreshPeriod

4cda752

Add external mapping CR changes

8a90ad7

CR fixes

1e78149

Merge remote-tracking branch 'origin/master' into add_external_mappin…

6b851e7

…g_cr_fixes

Fix build

bcf9ee2

Merge pull request #278 from logzio/add_external_mapping_cr_fixes_fixes

d96e8a6

Fix build

CR fixes

7d08b08

matvey-mtn requested a review from barakm March 2, 2022 17:46

barakm reviewed Mar 2, 2022

View reviewed changes

DanMelman reviewed Mar 3, 2022

View reviewed changes

CR fixes (#289)

15bfcf3

* CR fixes - added tests for escape characters - added test with empty values

matvey-mtn added 3 commits March 6, 2022 13:12

Merge remote-tracking branch 'origin/master' into add_external_mappin…

0951494

…g_source_pipeline

ExternalMappingSourceProcessor improve test coverage

d49bed1

ExternalMappingSourceProcessor CR Fix

aa4c78b

matvey-mtn requested review from barakm and DanMelman March 6, 2022 11:15

DanMelman reviewed Mar 6, 2022

View reviewed changes

DanMelman approved these changes Mar 6, 2022

View reviewed changes

barakm approved these changes Mar 6, 2022

View reviewed changes

matvey-mtn added 2 commits March 6, 2022 15:04

yet another fix

a3f9ba4

remove redundant import

7cfb7af

matvey-mtn merged commit 8933f13 into master Mar 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ExternalMappingSourceProcessor #271

Add ExternalMappingSourceProcessor #271

matvey-mtn commented Mar 1, 2022

barakm Mar 2, 2022

barakm Mar 2, 2022

matvey-mtn Mar 2, 2022

barakm Mar 2, 2022

barakm Mar 2, 2022

matvey-mtn Mar 2, 2022

barakm Mar 2, 2022

barakm Mar 2, 2022

matvey-mtn Mar 2, 2022

barakm Mar 2, 2022

matvey-mtn Mar 2, 2022 •

edited

barakm Mar 2, 2022

matvey-mtn Mar 3, 2022

barakm Mar 2, 2022

matvey-mtn Mar 3, 2022

DanMelman left a comment

matvey-mtn commented Mar 3, 2022

DanMelman commented Mar 3, 2022

DanMelman commented Mar 3, 2022

matvey-mtn commented Mar 4, 2022

DanMelman commented Mar 4, 2022

matvey-mtn commented Mar 4, 2022

DanMelman Mar 6, 2022

DanMelman Mar 6, 2022

DanMelman left a comment

	if (conn.getResponseCode() != 200) {
	if (conn.getResponseCode() != HttpURLConnection.HTTP_OK) {

	public Map<String, List<String>> getMappings() {
	public Map<String, List<String>> loadMappings() {


		public class ExternalMappingsClient {

		private final Logger logger = LoggerFactory.getLogger(ExternalMappingsClient.class);

Add ExternalMappingSourceProcessor #271

Add ExternalMappingSourceProcessor #271

Conversation

matvey-mtn commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matvey-mtn Mar 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanMelman left a comment

Choose a reason for hiding this comment

matvey-mtn commented Mar 3, 2022

DanMelman commented Mar 3, 2022

DanMelman commented Mar 3, 2022

matvey-mtn commented Mar 4, 2022

DanMelman commented Mar 4, 2022

matvey-mtn commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanMelman left a comment

Choose a reason for hiding this comment

matvey-mtn Mar 2, 2022 •

edited