Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5889] Bmoric/log application name for all workers [2/2] #7268

Merged
merged 41 commits into from
Oct 28, 2021

Conversation

benmoriceau
Copy link
Contributor

What

This is adding the application name in the logs produced by the docker containers.

This is related to #5889.

@benmoriceau benmoriceau added area/platform issues related to the platform 2021-q4-platform labels Oct 21, 2021
@github-actions github-actions bot added the area/worker Related to worker label Oct 21, 2021
@benmoriceau benmoriceau changed the base branch from master to bmoric/add-application-name October 21, 2021 23:55
Copy link
Contributor

@jrhizor jrhizor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Will wait to do a full review until the scope of application logging is solidified.

@benmoriceau benmoriceau temporarily deployed to more-secrets October 22, 2021 00:26 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets October 22, 2021 00:35 Inactive
@michel-tricot
Copy link
Contributor

Just a random idea. could we inject the name through a property instead and just have the log4j pattern to use that property?

@benmoriceau
Copy link
Contributor Author

benmoriceau commented Oct 22, 2021

Just a random idea. could we inject the name through a property instead and just have the log4j pattern to use that property?

@michel-tricot , This is actually what we do here, I splitted the review into 3 different ones. The first one is doing what you are suggesting. With the removal the log in the Source and destination for example, it makes less sense to have the complicated system. My original thought was to be able to have something like:

  • sync - log one
  • source - log 2
  • dest - log 3
  • sync - log 4

So even if you're runningin the same application, you know exactly where the log comes from.

The main idea was also to have a system in place to be able to add more information in the logs.

@benmoriceau benmoriceau temporarily deployed to more-secrets October 22, 2021 17:23 Inactive
@benmoriceau benmoriceau changed the base branch from bmoric/add-application-name to bmoric/mdc-manipulation-tools October 22, 2021 18:55
@benmoriceau benmoriceau temporarily deployed to more-secrets October 26, 2021 17:06 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets October 26, 2021 18:18 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets October 26, 2021 20:06 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets October 27, 2021 00:26 Inactive
@benmoriceau benmoriceau marked this pull request as ready for review October 27, 2021 00:33
@benmoriceau benmoriceau temporarily deployed to more-secrets October 27, 2021 20:48 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets October 27, 2021 21:24 Inactive
@@ -28,6 +31,10 @@

private static final Logger LOGGER = LoggerFactory.getLogger(DbtTransformationRunner.class);
private static final String DBT_ENTRYPOINT_SH = "entrypoint.sh";
private static final MdcScope CONTAINER_LOG_MDC = new Builder()
.setLogPrefix("dbt-container-log")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.setLogPrefix("dbt-container-log")
.setLogPrefix("dbt")

@@ -27,6 +30,10 @@
public class DefaultNormalizationRunner implements NormalizationRunner {

private static final Logger LOGGER = LoggerFactory.getLogger(DefaultNormalizationRunner.class);
private static final MdcScope CONTAINER_LOG_MDC = new Builder()
.setLogPrefix("normalization-container-log")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.setLogPrefix("normalization-container-log")
.setLogPrefix("normalization")

@@ -30,6 +33,10 @@
public class DefaultAirbyteDestination implements AirbyteDestination {

private static final Logger LOGGER = LoggerFactory.getLogger(DefaultAirbyteDestination.class);
private static final MdcScope CONTAINER_LOG_MDC = new Builder()
.setLogPrefix("destination-container-log")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.setLogPrefix("destination-container-log")
.setLogPrefix("destination")

@@ -35,6 +38,11 @@
private static final Duration GRACEFUL_SHUTDOWN_DURATION = Duration.of(10, ChronoUnit.HOURS);
private static final Duration FORCED_SHUTDOWN_DURATION = Duration.of(1, ChronoUnit.MINUTES);

private static final MdcScope CONTAINER_LOG_MDC = new Builder()
.setLogPrefix("source-container-log")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.setLogPrefix("source-container-log")
.setLogPrefix("source")

@benmoriceau benmoriceau temporarily deployed to more-secrets October 27, 2021 21:56 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets October 27, 2021 21:59 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets October 27, 2021 22:06 Inactive
@benmoriceau benmoriceau merged commit 9d30bb0 into master Oct 28, 2021
@benmoriceau benmoriceau deleted the bmoric/log-application-name branch October 28, 2021 00:03
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
)

This is adding the application name in the logs produced by the docker containers.

This is related to airbytehq#5889.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants