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

added filename and line number to logs #2637

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lslusarczyk
Copy link
Contributor

@lslusarczyk lslusarczyk commented Jan 29, 2025

Added an option to print line number and filename in log file. When fileline:1 is specified in an environment variable describing log, e.g:

UR_LOG_LEVEL_ZERO="level:debug;flush:warning;fileline:1;output:file,juju-l0.log"

then instead of seeing logs like this

<LEVEL_ZERO>[DEBUG]: create command list ..., inOrder: 1

one will see log like this:

<LEVEL_ZERO>[DEBUG]: create command list ..., inOrder: 1 <source/adapters/level_zero/v2/command_list_cache.cpp:73>

Review seems to be big but in fact is much shorter. The only essential change are some changes in

The rest was automated change of log statements from logger::xxx(...) to URLOG(XXX, ...)

@github-actions github-actions bot added loader Loader related feature/bug common Changes or additions to common utilities images UR images level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues opencl OpenCL adapter specific issues native-cpu Native CPU adapter specific issues command-buffer Command Buffer feature addition/changes/specification labels Jan 29, 2025
@lslusarczyk lslusarczyk marked this pull request as ready for review January 30, 2025 08:56
@lslusarczyk lslusarczyk requested review from a team as code owners January 30, 2025 08:56
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

I don't know... This might be adding a lot of usually not needed noise to messages. I feel like this should be an option.


#define URLOG_ALWAYS_IMPL(format, ...) \
{ ::logger::get_logger().always(format " <{}:{}>", __VA_ARGS__); }
#define URLOG_ALWAYS(...) URLOG_ALWAYS_IMPL(__VA_ARGS__, SHORT_FILE, __LINE__)
Copy link
Contributor

@pbalcer pbalcer Jan 30, 2025

Choose a reason for hiding this comment

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

why a macro and not a function?
nvm, I didn't look at the code. We'd need __LINE__...

Copy link
Contributor Author

@lslusarczyk lslusarczyk Jan 30, 2025

Choose a reason for hiding this comment

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

This has to be a macro because we need __LINE__ and __FILE__ from the place of log statement, not from the place of of function definition.

@lslusarczyk
Copy link
Contributor Author

lslusarczyk commented Jan 30, 2025

I don't know... This might be adding a lot of usually not needed noise to messages. I feel like this should be an option.

My opinion is that filename and line number is crucial for efficient log analysis, unless you work for a long time in a project and perfectly know the code. When I realized UR don't have it I feel it is a must - hence the PR.

Still when writing a log statement one can write it in old way and in this case log statement will appear without line and number.

@pbalcer
Copy link
Contributor

pbalcer commented Feb 4, 2025

I don't know... This might be adding a lot of usually not needed noise to messages. I feel like this should be an option.

My opinion is that filename and line number is crucial for efficient log analysis, unless you work for a long time in a project and perfectly know the code. When I realized UR don't have it I feel it is a must - hence the PR.

Still when writing a log statement one can write it in old way and in this case log statement will appear without line and number.

Many of the logs, especially error/warning ones, aren't meant for developers, but for users. Anyway, I'm not opposed, but I think this should be an option.


#define URLOG_IMPL(level, format, ...) \
{ \
::logger::get_logger().log(logger::Level::level, format " <{}:{}>", \
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of appending the extra information in the macro, instead you might be able to pass it as extra arguments to the log method, and print it out conditionally inside of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied

@igchor
Copy link
Member

igchor commented Feb 4, 2025

@lslusarczyk I think some of the SYCL tests might rely on those log messages being in a specific format. I think you should either make this an option (like Piotr said) or run the tests and see if there are no failures.

@github-actions github-actions bot added the sanitizer Sanitizer layer issues/changes/specification label Feb 10, 2025
@lslusarczyk
Copy link
Contributor Author

@lslusarczyk I think some of the SYCL tests might rely on those log messages being in a specific format. I think you should either make this an option (like Piotr said) or run the tests and see if there are no failures.

@igchor , @pbalcer I've applied this suggestion. Now behavior does not change unless one will specify "fileline:1" when configuring logs via environment variable.

@lslusarczyk lslusarczyk marked this pull request as draft February 10, 2025 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command-buffer Command Buffer feature addition/changes/specification common Changes or additions to common utilities cuda CUDA adapter specific issues hip HIP adapter specific issues images UR images level-zero L0 adapter specific issues loader Loader related feature/bug native-cpu Native CPU adapter specific issues opencl OpenCL adapter specific issues sanitizer Sanitizer layer issues/changes/specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants