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.

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

Jump to bottom

Reduce the log noise caused by core report summary #1426

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Reduce the log noise caused by core report summary #1426

merged 1 commit into from
Nov 19, 2024

Conversation

amahussein
Copy link
Collaborator

Signed-off-by: Ahmed Hussein (amahussein) [email protected]

Contributes to #1416

This change set the log level of ToolTextFileWriter to debug and summarize the logging into a single message to show the final directory

example output after the changes:

Qual Tool Progress 100% [=============================================] (1 succeeded + 0 failed + 0 skipped + 0 N/A) / 1
Qual Tool execution time: 1018ms
	process.success.count = 1
	process.failure.count = 0
	process.skipped.count = 0
	process.NA.count = 0
	execution.total.count = 1
24/11/15 20:43:35 INFO AutoTuner: Available memory per exec is not specified
24/11/15 20:43:35 INFO SuccessAppResult: File: file:/eventlogs/cpu/local-1731613821387, Message: Took 1001ms to process
APPLICATION SUMMARY:
================================================================================================================================================================
| App Name|             App ID|App Duration|SQL DF Duration|GPU Opportunity|        Unsupported Execs|Unsupported Expressions|Estimated Job Frequency (monthly)|
================================================================================================================================================================
|sqlmetric|local-1731613821387|        2721|            557|            436|Scan;SerializeFromObje...|                       |                               30|
================================================================================================================================================================
PER SQL SUMMARY:
==========================================================================================================================
| App Name|             App ID|Root SQL ID|SQL ID|                        SQL Description|SQL DF Duration|GPU Opportunity|
==========================================================================================================================
|sqlmetric|local-1731613821387|          5|     5|collect at SqlPlanParserSuite.scala:543|            328|            239|
|sqlmetric|local-1731613821387|          0|     0|collect at SqlPlanParserSuite.scala:537|            439|            146|
|sqlmetric|local-1731613821387|          1|     1|collect at SqlPlanParserSuite.scala:538|             82|             82|
|sqlmetric|local-1731613821387|          2|     2|collect at SqlPlanParserSuite.scala:540|             89|             30|
|sqlmetric|local-1731613821387|          4|     4|collect at SqlPlanParserSuite.scala:542|             39|              8|
|sqlmetric|local-1731613821387|          6|     6|      collect at ToolTestUtils.scala:99|             25|              8|
|sqlmetric|local-1731613821387|          3|     3|collect at SqlPlanParserSuite.scala:541|             20|              0|
==========================================================================================================================
24/11/15 20:43:35 INFO Qualification: Tools output directory: file:///output_folder/qual_20241114202203_3dfECa03-debug/rapids_4_spark_qualification_output

Process finished with exit code 0

@amahussein
Reduce the log noise caused by core report summary
26f0ab4 
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>

Contributes to NVIDIA#1416

This change set the log level of ToolTextFileWriter to debug and
summarize the logging into a single message to show the final directory
@amahussein amahussein added the core_tools Scope the core module (scala) label Nov 15, 2024
@amahussein amahussein self-assigned this Nov 15, 2024
@amahussein amahussein mentioned this pull request Nov 15, 2024
Open
@amahussein amahussein requested a review from parthosa November 15, 2024 21:01
parthosa
parthosa approved these changes Nov 15, 2024
View reviewed changes
Copy link
Collaborator

@parthosa parthosa left a comment

Choose a reason for hiding this comment

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

Thanks, @amahussein. LGTME. One minor comment.

core/src/main/scala/com/nvidia/spark/rapids/tool/ToolTextFileWriter.scala
@@ -70,7 +70,7 @@ class ToolTextFileWriter(
// No need to close the outputStream.
// Java should handle nested streams automatically.
utf8Writer.foreach { writer =>
logInfo(s"$finalLocationText output location: $textOutputLoc")
logDebug(s"$finalLocationText output location: $textOutputLoc")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be unnecessary but should we consider adding a single INFO level log per app indicating successful output generation, instead of logging for each individual file?

Again, it is unnecessary because these metrics are primarily for our internal use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if I get your comment.
Do you mean some sort of a summary of files that were generated but put them in a single message?

  • If that's the case, then I think we will hit the same issue that we have on the python side and Qualx. For example, when we generated tree of the output; it became too long. So, I believe if we put a single LogInfo message that contains list of all metrics generated, then the list will keep growing and I don't see that very useful information to put in the logging message.
  • In that change I changed the debug-level so that it does not get generated by default in production.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking of a single log line like:

INFO: Successfully generated output files for app <app-id> at <metrics location dir>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! For now this PR does exactly that.
We can later improve the implementation if we want the debug-level to be a single message as well.

@cindyyuanjiang cindyyuanjiang self-requested a review November 15, 2024 22:25
cindyyuanjiang
cindyyuanjiang approved these changes Nov 15, 2024
View reviewed changes
Copy link
Collaborator

@cindyyuanjiang cindyyuanjiang left a comment

Choose a reason for hiding this comment

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

Thanks @amahussein! QQ: this is only for switching the printing of output file names to debug, is that right?

@amahussein amahussein merged commit 9cba927 into NVIDIA:dev Nov 19, 2024
15 checks passed
@amahussein amahussein deleted the rapids-tools-1416-part1 branch November 19, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parthosa parthosa parthosa approved these changes

@cindyyuanjiang cindyyuanjiang cindyyuanjiang approved these changes

Assignees

@amahussein amahussein

Labels
core_tools Scope the core module (scala)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amahussein @parthosa @cindyyuanjiang