[5889] Bmoric/add application name [2/3] #7267

benmoriceau · 2021-10-21T22:43:04Z

What

Introduce a new interface which will allow to get an application name.

It will then be use in #5889 to log which application produce a log.

benmoriceau · 2021-10-21T22:43:41Z

I have added 2 reviewers since it is a potentially impactful change.

jrhizor · 2021-10-21T22:57:38Z

airbyte-workers/src/main/java/io/airbyte/workers/DefaultCheckConnectionWorker.java

@@ -86,4 +86,7 @@ public void cancel() {
    WorkerUtils.cancelProcess(process);
  }

+  @Override public String getApplicationName() {
+    return "airbyte-connection-checker";


Suggested change

return "airbyte-connection-checker";

return "check-connection-worker";

jrhizor · 2021-10-21T22:57:50Z

airbyte-workers/src/main/java/io/airbyte/workers/DefaultDiscoverCatalogWorker.java

@@ -81,4 +81,7 @@ public void cancel() {
    WorkerUtils.cancelProcess(process);
  }

+  @Override public String getApplicationName() {
+    return "airbyte-catalog-discovery";


Suggested change

return "airbyte-catalog-discovery";

return "discover-worker";

jrhizor · 2021-10-21T22:58:13Z

airbyte-workers/src/main/java/io/airbyte/workers/DefaultGetSpecWorker.java

@@ -81,4 +81,7 @@ public void cancel() {
    WorkerUtils.cancelProcess(process);
  }

+  @Override public String getApplicationName() {
+    return "airbyte-spec-getter";


Suggested change

return "airbyte-spec-getter";

return "get-spec-worker";

jrhizor · 2021-10-21T22:58:25Z

airbyte-workers/src/main/java/io/airbyte/workers/DefaultNormalizationWorker.java

@@ -82,4 +82,7 @@ public void cancel() {
    }
  }

+  @Override public String getApplicationName() {
+    return "airbyte-normalization";


Suggested change

return "airbyte-normalization";

return "normalization-worker";

jrhizor · 2021-10-21T22:58:52Z

airbyte-workers/src/main/java/io/airbyte/workers/DbtTransformationWorker.java

@@ -77,4 +77,7 @@ public void cancel() {
    }
  }

+  @Override public String getApplicationName() {
+    return "airbyte-dbt-transformation-worker";


Suggested change

return "airbyte-dbt-transformation-worker";

return "dbt-worker";

jrhizor · 2021-10-21T23:01:35Z

...te-workers/src/main/java/io/airbyte/workers/protocols/airbyte/DefaultAirbyteDestination.java

@@ -142,4 +142,7 @@ public boolean isFinished() {
    return Optional.ofNullable(messageIterator.hasNext() ? messageIterator.next() : null);
  }

+  @Override public String getApplicationName() {
+    return "airbyte-destination";


Even this level is a bit misleading. To a user in the UI, "destination" would likely make it seem like something running on the destination pod/container, not the destination class within the sync worker.

👍 I have removed it.

jrhizor · 2021-10-21T23:01:44Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/DefaultAirbyteSource.java

@@ -129,4 +128,7 @@ public void cancel() throws Exception {
    }
  }

+  @Override public String getApplicationName() {


jrhizor · 2021-10-21T23:01:53Z

...-workers/src/main/java/io/airbyte/workers/protocols/airbyte/DefaultAirbyteStreamFactory.java

@@ -89,4 +87,7 @@ private void internalLog(final AirbyteLogMessage logMessage) {
    }
  }

+  @Override public String getApplicationName() {


jrhizor · 2021-10-21T23:02:10Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/EmptyAirbyteSource.java

@@ -56,4 +55,7 @@ public void cancel() throws Exception {
    // no op.
  }

+  @Override public String getApplicationName() {


jrhizor · 2021-10-21T23:02:14Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/NamespacingMapper.java

@@ -97,4 +96,7 @@ private static String transformStreamName(final String streamName, final String
    }
  }

+  @Override public String getApplicationName() {


…airbyte into bmoric/add-application-name

michel-tricot · 2021-10-22T00:22:05Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/MessageTracker.java

 import java.util.Optional;
 import java.util.function.Consumer;

-public interface MessageTracker<T> extends Consumer<T> {
+public interface MessageTracker<T> extends Consumer<T>, Application {


this doesn't seem right. Message Tracker isn't an Application. I wonder if the way to track name is wrong or just the naming of this interface. WDYT?

I think that we need make sure that I properly understand how granular we want to log. My initial thought was that we have a different prefix within the same application (a different prefix per step). So if we want to go in this direction we will probably need to rename the interface. If not I should remove the Application interface and move this method to the worker interface.

I agree the iface name should change.

Will follow up offline, but I'm am not understanding the granularity we are going for right now. The issue is focused on distinguishing source, destination, and platform logs from each other. This PR is focused on distinguishing worker logs from each other, and I'm a little less clear on why that is valuable.

I sync up offline with Michel on that. I will abandon the interface and this review for something more suited to what is needed.

…airbyte into bmoric/add-application-name

cgardens

a couple questions on the scope of what we are doing here. going to follow up offline.

cgardens · 2021-10-22T17:37:17Z

airbyte-workers/src/main/java/io/airbyte/workers/Application.java

+  default String getApplicationName() {
+    // This value should only be used in the U-Test, it is an empty string instead of airbyte-test in
+    // order to avoid displaying airbyte-test in prod
+    return "";
+  }


By convention, we do not usually use a default in an interface if the default implementation is for testing only. The reason for this is that with this default, it is easy for a developer to make a mistake using this interface because the compiler won't warn them that they need to override method. If they fail to do that, then test code will accidentally run in production. Instead we would usually structure it something like this, so that the compiler complains if the developer makes a mistake.

Suggested change

default String getApplicationName() {

// This value should only be used in the U-Test, it is an empty string instead of airbyte-test in

// order to avoid displaying airbyte-test in prod

return "";

}

String getApplicationName();

class TestApplication implements Application {

@Override

public String getApplicationName() {

// This value should only be used in the U-Test, it is an empty string instead of airbyte-test in

// order to avoid displaying airbyte-test in prod

return "";

}

}

cgardens · 2021-10-22T18:05:17Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/MessageTracker.java

 import java.util.Optional;
 import java.util.function.Consumer;

-public interface MessageTracker<T> extends Consumer<T> {
+public interface MessageTracker<T> extends Consumer<T>, Application {


I agree the iface name should change.

Will follow up offline, but I'm am not understanding the granularity we are going for right now. The issue is focused on distinguishing source, destination, and platform logs from each other. This PR is focused on distinguishing worker logs from each other, and I'm a little less clear on why that is valuable.

Add an application name

3b1a65a

benmoriceau added type/refactoring Focused on refactoring. area/platform issues related to the platform 2021-q4-platform labels Oct 21, 2021

benmoriceau requested review from jrhizor and cgardens October 21, 2021 22:43

github-actions bot added the area/worker Related to worker label Oct 21, 2021

benmoriceau changed the base branch from master to bmoric/mdc-manipulation-tools October 21, 2021 22:45

jrhizor suggested changes Oct 21, 2021

View reviewed changes

Merge branch 'bmoric/mdc-manipulation-tools' of github.com:airbytehq/…

dd8a123

…airbyte into bmoric/add-application-name

benmoriceau temporarily deployed to more-secrets October 22, 2021 00:06 Inactive

PR Comments

20964f0

benmoriceau temporarily deployed to more-secrets October 22, 2021 00:14 Inactive

michel-tricot reviewed Oct 22, 2021

View reviewed changes

benmoriceau added 2 commits October 21, 2021 17:31

Merge branch 'bmoric/mdc-manipulation-tools' of github.com:airbytehq/…

bd4a7ff

…airbyte into bmoric/add-application-name

Format

ee6b7e9

benmoriceau temporarily deployed to more-secrets October 22, 2021 00:33 Inactive

cgardens requested changes Oct 22, 2021

View reviewed changes

benmoriceau closed this Oct 22, 2021

swyxio deleted the bmoric/add-application-name branch October 12, 2022 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5889] Bmoric/add application name [2/3] #7267

[5889] Bmoric/add application name [2/3] #7267

benmoriceau commented Oct 21, 2021

benmoriceau commented Oct 21, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

jrhizor Oct 21, 2021

benmoriceau Oct 22, 2021

michel-tricot Oct 22, 2021 •

edited

Loading

benmoriceau Oct 22, 2021

cgardens Oct 22, 2021

benmoriceau Oct 22, 2021 •

edited

Loading

cgardens left a comment

cgardens Oct 22, 2021

cgardens Oct 22, 2021

	return "airbyte-connection-checker";
	return "check-connection-worker";

	return "airbyte-catalog-discovery";
	return "discover-worker";

	return "airbyte-normalization";
	return "normalization-worker";

	return "airbyte-dbt-transformation-worker";
	return "dbt-worker";

[5889] Bmoric/add application name [2/3] #7267

[5889] Bmoric/add application name [2/3] #7267

Conversation

benmoriceau commented Oct 21, 2021

What

benmoriceau commented Oct 21, 2021

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

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

michel-tricot Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmoriceau Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michel-tricot Oct 22, 2021 •

edited

Loading

benmoriceau Oct 22, 2021 •

edited

Loading