Issue 288: Add log messages #289
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
==========================================
- Coverage 90.78% 90.77% -0.01%
==========================================
Files 98 98
Lines 2604 2623 +19
Branches 346 336 -10
==========================================
+ Hits 2364 2381 +17
- Misses 240 242 +2 ☔ View full report in Codecov by Sentry. |
|
With the recent commit, the output looks like this: Note that I have configured log4j to only output Qbeast log messages with severity level TRACE, the severity level for Spark remains as WARN. |
…ng to a table with qbeast
fpj
force-pushed
the
288-add-log-messages
branch
from
ccee887 to
5c3311b
Compare
osopardo1
reviewed
Apr 3, 2024
fpj
commented
Apr 3, 2024
There was a problem hiding this comment.
Thanks for the comments, @osopardo1 . Check my responses when you have a chance, pls.
…ments and some further polishing of log messages
|
On the questions you asked earlier:
Roughly how I use them:
We should consider having more formal definitions with examples as part of contribution guidelines.
It is a good point. If the code path where the log message lies is being executed, then the corresponding log method (logDebug, logInfo) is also being executed. But, it is accurate that a test case is not necessarily going to spot whether a message is not useful for a human being or even some automation software that processes logs. If there are downstream systems consuming such logs, then we could consider having integration tests to validate the messages. |
Got it! Let me write the first point into an issue. The test reasoning seems good to me. But now seems like Codecov is not executing those lines... I don't know what is the problem exactly, I am not able to detect a pattern (if it's not printing trace or debug...) and the other parts of the code are being tested as well. Do you know what can be? |
|
Aside from Codecov complains, the PR LGTM 🙌 |
osopardo1
approved these changes
Apr 4, 2024
|
@osopardo1 here is a sample output: For this command: |
…riter and OTreeDataAnalyzer, a couple of additional capitalization fixes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
It adds log messages to be able to trace activity when executing such as the following:
The code at the moment is very scarce on log messages, in factr, I couldn't find any in the part of the code I was inspecting. It is going to take multiple PRs to populate the code base with logging messages appropriately.
Fixes #288
Type of change
It is adding log messages using the spark internal logging trait. As such, we have to add a line to specific classes to have them using
org.apache.spark.internal.Logging.In the log messages I added so far, I have prefixed it with
Qbeast:to make it easier to locate the messages among all INFO messages that Spark generated. That's not ideal, and I'd much rather have the logs printing the fully-qualified class name. We might want to change it before merging.Checklist:
Here is the list of things you should do before submitting this pull request:
How Has This Been Tested? (Optional)
I have validated locally with
spark-shellthat the messages are printing.Test Configuration: