You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/06/03 23:26:29 UTC
[GitHub] [flink] dasbh opened a new pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
dasbh opened a new pull request #12472:
URL: https://github.com/apache/flink/pull/12472
## What is the purpose of the change
* This pull request makes task instance to have jobName at the instance level and use the same for to populate MDC
## Problem statement:
We use a shared session cluster (Standalone/Yarn) to execute jobs. All logs from the cluster are shipped using log aggregation framework (Logstash/Splunk) so that application diagnostic is easier.
However, we are missing one vital information in the logline. i.e. Job name so that we can filter the logs for a single job.
## Brief change log
- *The Task class now includes jobName filed, populated from JobInformation on construction, similar to jobId*
- *When TaskExecutor runs Task, run method populates the jobName in MDC so that it can be used in logging pattern as %X{jobName}*
- *ExecutorThreadFactory passes MDC context down to worker threads by wrapping runnable so MDC can be safely used on Cached Thread pool. This is the recommended way to pass the MDC from parent to Executor thread pool*
## Verifying this change
This change added tests and can be verified as follows:
- *Added unit test for Task which verifies jobName and MDC *
- *Manually verified logging by changing WordCount Example to include %X{jobName} *
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): don't know
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? yes
- If yes, how is the feature documented? not documented yet
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a change in pull request #12472: [FLINK-17969] Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #12472:
URL: https://github.com/apache/flink/pull/12472#discussion_r538238119
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
##########
@@ -530,8 +539,10 @@ public void startTaskThread() {
@Override
public void run() {
try {
+ MDC.put("jobName", this.jobName);
Review comment:
Maybe this should also be configurable to use the job ID.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] pnowojski commented on a change in pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
pnowojski commented on a change in pull request #12472:
URL: https://github.com/apache/flink/pull/12472#discussion_r512001282
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/ExecutorThreadFactory.java
##########
@@ -106,6 +109,8 @@ public ExecutorThreadFactory(String poolName, UncaughtExceptionHandler exception
@Override
public Thread newThread(Runnable runnable) {
+ runnable = new RunnableMDCWrapper(runnable);
+
Review comment:
Will this work correctly if the `ExecutorThreadFactory` is used in some thread pool that is shared between different jobs?
I think at best, we would loose the `MDC`. At worst, if new pooled worker is spawned when in the scope of Job1's MDC, this MDC would stick with this worker forever, regardless if it's executing code belonging to some another job?
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
##########
@@ -530,8 +539,10 @@ public void startTaskThread() {
@Override
public void run() {
try {
+ MDC.put("jobName", this.jobName);
doRun();
} finally {
+ MDC.remove("jobName");
terminationFuture.complete(executionState);
Review comment:
Don't we want to have the same changes in `LegacySourceFunctionThread` as well?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] ricardopereira33 edited a comment on pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
ricardopereira33 edited a comment on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-705698142
Hi @rmetzger 👋
Do you have any idea when this PR will be reviewed? I'm working in a Flink Cluster where there are several jobs running and it is hard to track/debug a specific Job (the logs are all mixed up). At least, it would be nice to know if you are planning to add this feature in the next release 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] ricardopereira33 commented on pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
ricardopereira33 commented on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-705698142
Hi @rmetzger 👋
Are you any idea when this PR will be reviewed? I'm working in a Flink Cluster where there are several jobs running and it is hard to track/debug a specific Job (the logs are all mixed up). At least, it would be nice to know if you are planning to add this feature in the next release 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a change in pull request #12472: [FLINK-17969] Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #12472:
URL: https://github.com/apache/flink/pull/12472#discussion_r537503947
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
##########
@@ -530,8 +539,10 @@ public void startTaskThread() {
@Override
public void run() {
try {
+ MDC.put("jobName", this.jobName);
Review comment:
We may want to introduce an opt-in configuration option. I cannot assess how many threads are being created in existing jobs, and the copy of the context map makes be a bit wary.
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskTest.java
##########
@@ -69,10 +71,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
-import java.util.concurrent.BlockingQueue;
-import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.LinkedBlockingDeque;
-import java.util.concurrent.TimeoutException;
+import java.util.concurrent.*;
Review comment:
we don't allow `*` imports
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
##########
@@ -530,8 +539,10 @@ public void startTaskThread() {
@Override
public void run() {
try {
+ MDC.put("jobName", this.jobName);
doRun();
} finally {
+ MDC.remove("jobName");
terminationFuture.complete(executionState);
Review comment:
Depends a bit. From what I saw in the log4j code, by default the thread context is not inherited to child threads, so we would actually need to setup the MDC whenever any thread is created, which is naturally terrible.
If the `isThreadContextMapInheritable` property is set to `true`, then all threads created within a thread inherit the MDC (as a copy), and then we shouldn't need to do anything.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/ExecutorThreadFactory.java
##########
@@ -106,6 +109,8 @@ public ExecutorThreadFactory(String poolName, UncaughtExceptionHandler exception
@Override
public Thread newThread(Runnable runnable) {
+ runnable = new RunnableMDCWrapper(runnable);
+
Review comment:
I think so, because the wrapper resets the MDC after it has finished.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/ExecutorThreadFactory.java
##########
@@ -106,6 +109,8 @@ public ExecutorThreadFactory(String poolName, UncaughtExceptionHandler exception
@Override
public Thread newThread(Runnable runnable) {
+ runnable = new RunnableMDCWrapper(runnable);
+
Review comment:
That said, we shouldn't just override the MDC of the thread; if the owner of the pool (e.g., TM) has something in the MDC then it should be preserved. (i.e., merge the MDCs instead)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on pull request #12472: [FLINK-17969] Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
zentol commented on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-757906268
I will close the PR for the time being. It appears to be abandoned, and I think we have to look more into how the MDC could work on the Dispatcher side of things as well as the overall performance impact.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol closed pull request #12472: [FLINK-17969] Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
zentol closed pull request #12472:
URL: https://github.com/apache/flink/pull/12472
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12472: [FLINK-17969] Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-638513120
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit 79160714a0666d5c73ab75fa91eef8d2b6d29396 (Fri May 28 07:04:00 UTC 2021)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
* **This pull request references an unassigned [Jira ticket](https://issues.apache.org/jira/browse/FLINK-17969).** According to the [code contribution guide](https://flink.apache.org/contributing/contribute-code.html), tickets need to be assigned before starting with the implementation work.
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-638513120
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit 79160714a0666d5c73ab75fa91eef8d2b6d29396 (Wed Jun 03 23:28:16 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
* **This pull request references an unassigned [Jira ticket](https://issues.apache.org/jira/browse/FLINK-17969).** According to the [code contribution guide](https://flink.apache.org/contributing/contribute-code.html), tickets need to be assigned before starting with the implementation work.
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] ricardopereira33 edited a comment on pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
ricardopereira33 edited a comment on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-705698142
Hi @rmetzger 👋
Do you have any idea when this PR will be reviewed? I'm working in a Flink Cluster where there are several jobs running and it is hard to track/debug a specific Job (the logs are all mixed up). At least, it would be nice to know if you are planning to add this feature in the next release 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-638519195
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "79160714a0666d5c73ab75fa91eef8d2b6d29396",
"status" : "PENDING",
"url" : "https://travis-ci.com/github/flink-ci/flink/builds/169510522",
"triggerID" : "79160714a0666d5c73ab75fa91eef8d2b6d29396",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 79160714a0666d5c73ab75fa91eef8d2b6d29396 Travis: [PENDING](https://travis-ci.com/github/flink-ci/flink/builds/169510522)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-638519195
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "79160714a0666d5c73ab75fa91eef8d2b6d29396",
"status" : "SUCCESS",
"url" : "https://travis-ci.com/github/flink-ci/flink/builds/169510522",
"triggerID" : "79160714a0666d5c73ab75fa91eef8d2b6d29396",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 79160714a0666d5c73ab75fa91eef8d2b6d29396 Travis: [SUCCESS](https://travis-ci.com/github/flink-ci/flink/builds/169510522)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] ricardopereira33 commented on pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
ricardopereira33 commented on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-705698142
Hi @rmetzger 👋
Are you any idea when this PR will be reviewed? I'm working in a Flink Cluster where there are several jobs running and it is hard to track/debug a specific Job (the logs are all mixed up). At least, it would be nice to know if you are planning to add this feature in the next release 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #12472: FLINK-17969 Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12472:
URL: https://github.com/apache/flink/pull/12472#issuecomment-638519195
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "79160714a0666d5c73ab75fa91eef8d2b6d29396",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "79160714a0666d5c73ab75fa91eef8d2b6d29396",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 79160714a0666d5c73ab75fa91eef8d2b6d29396 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a change in pull request #12472: [FLINK-17969] Enhance Flink logging to include job name as MDC
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #12472:
URL: https://github.com/apache/flink/pull/12472#discussion_r538238119
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
##########
@@ -530,8 +539,10 @@ public void startTaskThread() {
@Override
public void run() {
try {
+ MDC.put("jobName", this.jobName);
Review comment:
~~Maybe this should also be configurable to use the job ID.~~
This should also include the job ID, and maybe even the task id & name.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org