Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 11 additions & 3 deletions common/src/main/java/org/apache/comet/NativeBase.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

import static org.apache.comet.Constants.LOG_CONF_NAME;
import static org.apache.comet.Constants.LOG_CONF_PATH;
import static org.apache.comet.Constants.LOG_LEVEL_ENV;

/** Base class for JNI bindings. MUST be inherited by all classes that introduce JNI APIs. */
public abstract class NativeBase {
Expand Down Expand Up @@ -153,18 +154,25 @@ private static void bundleLoadLibrary() {

private static void initWithLogConf() {
String logConfPath = System.getProperty(LOG_CONF_PATH(), Utils.getConfPath(LOG_CONF_NAME()));
String logLevel = System.getenv(LOG_LEVEL_ENV());

// If both the system property and the environmental variable failed to find a log
// configuration, then fall back to using the deployed default
if (logConfPath == null) {
LOG.info(
"Couldn't locate log file from either COMET_CONF_DIR or comet.log.file.path. "
+ "Using default log configuration which emits to stdout");
+ "Using default log configuration with {} log level which emits to stderr",
Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected to emits to stderr

.target(Target::Stderr)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why change to stderr? Shouldn't the default log level should be info, emitting to stdout?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why change to stderr? Shouldn't the default log level should be info, emitting to stdout?

This is just a log fix, the behavior change was introduced by #164

logLevel == null ? "INFO" : logLevel);
logConfPath = "";
} else {
// Ignore log level if a log configuration file is specified
if (logLevel != null) {
LOG.warn("Ignoring log level {} because a log configuration file is specified", logLevel);
}

LOG.info("Using {} for native library logging", logConfPath);
}
init(logConfPath);
init(logConfPath, logLevel);
}

private static void cleanupOldTempLibs() {
Expand Down Expand Up @@ -283,7 +291,7 @@ private static String resourceName() {
*
* @param logConfPath location to the native log configuration file
*/
static native void init(String logConfPath);
static native void init(String logConfPath, String logLevel);

/**
* Check if a specific feature is enabled in the native library.
Expand Down
2 changes: 2 additions & 0 deletions common/src/main/scala/org/apache/comet/Constants.scala
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
package org.apache.comet

object Constants {
val COMET_CONF_DIR_ENV = "COMET_CONF_DIR"
val LOG_CONF_PATH = "comet.log.file.path"
val LOG_CONF_NAME = "log4rs.yaml"
val LOG_LEVEL_ENV = "COMET_LOG_LEVEL"
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,14 @@ import org.apache.spark.sql.types._
import org.apache.spark.sql.vectorized.ColumnarBatch
import org.apache.spark.util.io.{ChunkedByteBuffer, ChunkedByteBufferOutputStream}

import org.apache.comet.Constants.COMET_CONF_DIR_ENV
import org.apache.comet.shims.CometTypeShim
import org.apache.comet.vector.CometVector

object Utils extends CometTypeShim {
def getConfPath(confFileName: String): String = {
sys.env
.get("COMET_CONF_DIR")
.get(COMET_CONF_DIR_ENV)
.map { t => new File(s"$t${File.separator}$confFileName") }
.filter(_.isFile)
.map(_.getAbsolutePath)
Expand Down
28 changes: 28 additions & 0 deletions docs/source/contributor-guide/debugging.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ Detecting the debugger

## Verbose debug

### Exception details

By default, Comet outputs the exception details specific for Comet.

```scala
Expand Down Expand Up @@ -162,3 +164,29 @@ Note:

- The backtrace coverage in DataFusion is still improving. So there is a chance the error still not covered, if so feel free to file a [ticket](https://github.com/apache/arrow-datafusion/issues)
- The backtrace evaluation comes with performance cost and intended mostly for debugging purposes

### Native log configuration

By default, Comet emits native-side logs at the `INFO` level to `stderr`.

You can use the `COMET_LOG_LEVEL` environment variable to specify the log level. Supported values are: `OFF`, `ERROR`, `WARN`, `INFO`, `DEBUG`, `TRACE`.

For example, to configure native logs at the `DEBUG` level on spark executor:

```
spark.executorEnv.COMET_LOG_LEVEL=DEBUG
```

This produces output like the following:

```
25/09/15 20:17:42 INFO core/src/lib.rs: Comet native library version 0.11.0 initialized
25/09/15 20:17:44 DEBUG /xxx/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/datafusion-execution-49.0.2/src/disk_manager.rs: Created local dirs [TempDir { path: "/private/var/folders/4p/9gtjq1s10fd6frkv9kzy0y740000gn/T/blockmgr-ba524f95-a792-4d79-b49c-276ba324941e/datafusion-qrpApx" }] as DataFusion working directory
25/09/15 20:17:44 DEBUG /xxx/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/datafusion-functions-nested-49.0.2/src/lib.rs: Overwrite existing UDF: array_to_string
25/09/15 20:17:44 DEBUG /xxx/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/datafusion-functions-nested-49.0.2/src/lib.rs: Overwrite existing UDF: string_to_array
25/09/15 20:17:44 DEBUG /xxx/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/datafusion-functions-nested-49.0.2/src/lib.rs: Overwrite existing UDF: range
...
```

Additionally, you can place a `log4rs.yaml` configuration file inside the Comet configuration directory specified by the `COMET_CONF_DIR` environment variable to enable more advanced logging configurations. This file uses the [log4rs YAML configuration format](https://docs.rs/log4rs/latest/log4rs/#configuration-via-a-yaml-file).
For example, see: [log4rs.yaml](https://github.com/apache/datafusion-comet/blob/main/conf/log4rs.yaml).
19 changes: 14 additions & 5 deletions native/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use jni::{
objects::{JClass, JString},
JNIEnv, JavaVM,
};
use log::{info, LevelFilter};
use log::info;
use log4rs::{
append::console::{ConsoleAppender, Target},
config::{load_config_file, Appender, Deserializers, Root},
Expand Down Expand Up @@ -70,6 +70,7 @@ pub extern "system" fn Java_org_apache_comet_NativeBase_init(
e: JNIEnv,
_: JClass,
log_conf_path: JString,
log_level: JString,
) {
// Initialize the error handling to capture panic backtraces
errors::init();
Expand All @@ -80,7 +81,11 @@ pub extern "system" fn Java_org_apache_comet_NativeBase_init(
// empty path means there is no custom log4rs config file provided, so fallback to use
// the default configuration
let log_config = if path.is_empty() {
default_logger_config()
let log_level: String = match env.get_string(&log_level) {
Ok(level) => level.into(),
Err(_) => "info".parse().unwrap(),
};
default_logger_config(&log_level)
} else {
load_config_file(path, Deserializers::default())
.map_err(|err| CometError::Config(err.to_string()))
Expand Down Expand Up @@ -129,14 +134,18 @@ pub extern "system" fn Java_org_apache_comet_NativeBase_isFeatureEnabled(
})
}

// Creates a default log4rs config, which logs to console with `INFO` level.
fn default_logger_config() -> CometResult<Config> {
// Creates a default log4rs config, which logs to console with log level.
fn default_logger_config(log_level: &str) -> CometResult<Config> {
let console_append = ConsoleAppender::builder()
.target(Target::Stderr)
.encoder(Box::new(PatternEncoder::new(LOG_PATTERN)))
.build();
let appender = Appender::builder().build("console", Box::new(console_append));
let root = Root::builder().appender("console").build(LevelFilter::Info);
let root = Root::builder().appender("console").build(
log_level
.parse()
.map_err(|err| CometError::Config(format!("{err}")))?,
);
Config::builder()
.appender(appender)
.build(root)
Expand Down
Loading