Skip to content

[SPARK-14754][SPARK CORE] Metrics as logs are not coming through slf4j #12697

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.

Sign up for GitHub

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

Closed
mihir6692 wants to merge 4 commits into from
Closed

[SPARK-14754][SPARK CORE] Metrics as logs are not coming through slf4j #12697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12d896c
[SPARK-14754][CORE]Metrics as logs are not coming through slf4j
mihir6692 Apr 26, 2016
5a437a1
[SPARK-14754][CORE]Metrics as logs are not coming through slf4j
mihir6692 Apr 26, 2016
032422b
[SPARK-14754][CORE]Metrics as logs are not coming through slf4j
mihir6692 Apr 26, 2016
7f961e8
[SPARK-14754][CORE]Metrics as logs are not coming through slf4j
mihir6692 Apr 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions conf/log4j.properties.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,14 @@ log4j.logger.parquet=ERROR
# SPARK-9183: Settings to avoid annoying messages when looking up nonexistent UDFs in SparkSQL with Hive support
log4j.logger.org.apache.hadoop.hive.metastore.RetryingHMSHandler=FATAL
log4j.logger.org.apache.hadoop.hive.ql.exec.FunctionRegistry=ERROR

# SPARK-14754: Metrics as logs are not coming through slf4j.
#log4j.logger.org.apache.spark.metrics=INFO, metricFileAppender
#log4j.additivity.org.apache.spark.metrics=true

#log4j.appender.metricFileAppender=org.apache.log4j.RollingFileAppender
#log4j.appender.metricFileAppender.File=${logFilePath}
#log4j.appender.metricFileAppender.MaxFileSize=10MB
Copy link
Member

@srowen srowen Apr 26, 2016

Choose a reason for hiding this comment

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

Why is all this config needed?

Copy link
Member

@srowen srowen Apr 26, 2016

Choose a reason for hiding this comment

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

I still don't think this has been addressed. It shouldn't need a new appender, right?

Copy link
Author

@mihir6692 mihir6692 Apr 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

For log4j.properties :-

Main reason to use new appenders is to get metrics in separate file which
will reduce complexity if someone one wants to parse metric.

It would be better to use new appender so when you disable root logger ,
you will only disable application logs, not the metrics logs. ( for more
detailed and through explanation :- http://stackoverflow.com/a/23323046 )

On Tue, Apr 26, 2016 at 4:27 PM, Sean Owen notifications@github.com wrote:

In conf/log4j.properties.template
#12697 (comment):

@@ -38,3 +38,14 @@ log4j.logger.parquet=ERROR

SPARK-9183: Settings to avoid annoying messages when looking up nonexistent UDFs in SparkSQL with Hive support

log4j.logger.org.apache.hadoop.hive.metastore.RetryingHMSHandler=FATAL
log4j.logger.org.apache.hadoop.hive.ql.exec.FunctionRegistry=ERROR
+
+# SPARK-14754: Metrics as logs are not coming through slf4j.
+#log4j.logger.org.apache.spark.metrics=INFO, metricFileAppender
+#log4j.additivity.org.apache.spark.metrics=true
+
+#log4j.appender.metricFileAppender=org.apache.log4j.RollingFileAppender
+#log4j.appender.metricFileAppender.File=${logFilePath}
+#log4j.appender.metricFileAppender.MaxFileSize=10MB

I still don't think this has been addressed. It shouldn't need a new
appender, right?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/12697/files/5a437a12fb0c6cfdd7119d63bebc3f3f2c935d5f#r61066147

Mihir Monani
(+91)-9429473434

#log4j.appender.metricFileAppender.MaxBackupIndex=10
#log4j.appender.metricFileAppender.layout=org.apache.log4j.PatternLayout
#log4j.appender.metricFileAppender.layout.ConversionPattern=%d{yyyy-MM-dd HH:mm:ss} %-5p %c{1}:%L - %m%n
4 changes: 4 additions & 0 deletions core/src/main/scala/org/apache/spark/metrics/sink/Slf4jSink.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ import java.util.concurrent.TimeUnit

import com.codahale.metrics.{MetricRegistry, Slf4jReporter}

import org.slf4j.Logger
import org.slf4j.LoggerFactory
Copy link
Member

@HyukjinKwon HyukjinKwon Apr 26, 2016

Choose a reason for hiding this comment

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

I think we should reorder imports. (Please see databricks/scala-style-guide#imports.)

Copy link
Member

@HyukjinKwon HyukjinKwon Apr 26, 2016

Choose a reason for hiding this comment

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

(It would be great if you run sbt scalastyle before adding some more commits which shows up such things)

Copy link
Author

@mihir6692 mihir6692 Apr 26, 2016

Choose a reason for hiding this comment

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

I will run "sbt scalastyle" before adding some commits in future.

Regarding log4j.properties.template changes : -

I have added configuration template for metrics as log in
*log4j.properties.template,
*so other user doesn't face problem.

To Sean Owen :-

I don't understand what you meant by "Why not the class name for the logger
name here, per your JIRA?" ??

I gave generalize name for class as i didnt found any convention for it.

On Tue, Apr 26, 2016 at 2:37 PM, Hyukjin Kwon notifications@github.com
wrote:

In core/src/main/scala/org/apache/spark/metrics/sink/Slf4jSink.scala
#12697 (comment):

@@ -25,6 +25,9 @@ import com.codahale.metrics.{MetricRegistry, Slf4jReporter}
import org.apache.spark.SecurityManager
import org.apache.spark.metrics.MetricsSystem

+import org.slf4j.Logger
+import org.slf4j.LoggerFactory

(It would be great if you run sbt scalastyle before adding some more
commits which shows up such things)

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/12697/files/5a437a12fb0c6cfdd7119d63bebc3f3f2c935d5f#r61051439

Mihir Monani
(+91)-9429473434

Copy link
Member

@srowen srowen Apr 26, 2016

Choose a reason for hiding this comment

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

The problem you describe is that log messages aren't plumbed through to a logger, right? that shouldn't require changing appender config for log4j. Your JIRA says "Slf4jsink.scala should have class name for log4j to print metrics in log files" but your code has a package name.

Copy link
Author

@mihir6692 mihir6692 Apr 26, 2016

Choose a reason for hiding this comment

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

Ok Sean, I understood your point. I will recreate pull request with more
precise changes and will also run "sbt scalastyle" for it.

Should i close this pull request??

On Tue, Apr 26, 2016 at 2:48 PM, Sean Owen notifications@github.com wrote:

In core/src/main/scala/org/apache/spark/metrics/sink/Slf4jSink.scala
#12697 (comment):

@@ -25,6 +25,9 @@ import com.codahale.metrics.{MetricRegistry, Slf4jReporter}
import org.apache.spark.SecurityManager
import org.apache.spark.metrics.MetricsSystem

+import org.slf4j.Logger
+import org.slf4j.LoggerFactory

The problem you describe is that log messages aren't plumbed through to a
logger, right? that shouldn't require changing appender config for log4j.
Your JIRA says "Slf4jsink.scala should have class name for log4j to print
metrics in log files" but your code has a package name.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/12697/files/5a437a12fb0c6cfdd7119d63bebc3f3f2c935d5f#r61053012

Mihir Monani
(+91)-9429473434

Copy link
Member

@srowen srowen Apr 26, 2016

Choose a reason for hiding this comment

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

No you simply push more commits to this branch

Copy link
Author

@mihir6692 mihir6692 Apr 26, 2016

Choose a reason for hiding this comment

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

ok. I will push more commits and update pull request.
Thanks.

On Tue, Apr 26, 2016 at 2:52 PM, Sean Owen notifications@github.com wrote:

In core/src/main/scala/org/apache/spark/metrics/sink/Slf4jSink.scala
#12697 (comment):

@@ -25,6 +25,9 @@ import com.codahale.metrics.{MetricRegistry, Slf4jReporter}
import org.apache.spark.SecurityManager
import org.apache.spark.metrics.MetricsSystem

+import org.slf4j.Logger
+import org.slf4j.LoggerFactory

No you simply push more commits to this branch

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/12697/files/5a437a12fb0c6cfdd7119d63bebc3f3f2c935d5f#r61053629

Mihir Monani
(+91)-9429473434


import org.apache.spark.SecurityManager
import org.apache.spark.metrics.MetricsSystem

Expand Down Expand Up @@ -49,6 +52,7 @@ private[spark] class Slf4jSink(
MetricsSystem.checkMinimalPollingPeriod(pollUnit, pollPeriod)

val reporter: Slf4jReporter = Slf4jReporter.forRegistry(registry)
.outputTo(LoggerFactory.getLogger("org.apache.spark.metrics.sink.Slf4jSink"))
Copy link
Member

@srowen srowen Apr 26, 2016

Choose a reason for hiding this comment

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

Sure, it can be classOf[Slf4jSink].getName for simplicity and to avoid forgetting to update it if for some reason this moves.

Copy link
Author

@mihir6692 mihir6692 Apr 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

For Slf4jSink.scala :-

Its not about class path or class level in package. It is just a name. Ex.
If you keep name like Spark.log4j, it would still work. ( and use the
same Spark.log4j in log4j.properties). So it won't matter even if we
move class to some other folder or package.

EDIT :-

I understood what you are trying to say. I will update it with new commit. Thanks for guidance.

.convertDurationsTo(TimeUnit.MILLISECONDS)
.convertRatesTo(TimeUnit.SECONDS)
.build()
Expand Down