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

Clean up cruft and basic file logging setup #69

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stxue1
Copy link
Collaborator

@stxue1 stxue1 commented Aug 21, 2024

For logging we can use lombok's automation and Slf4j (I think slf4j is better than apache CommonsLog, both can act as bridges for logging implementations but slf4j seems to be more "robust" in their own words)

It'll look something like this:

@Slf4j
public class TestClass {
    public static void some_function() {
        log.debug("hello world");
    }
}

Springboot uses logback by default which will connect to the Slf4j abstraction. The advantage is if we want to migrate to log4j2 (which is supposed to be the successor to logback and is more complicated) it should be drag and drop.

We also should figure out how to properly use exceptions; it does not make sense to raise a RuntimeException when a function fails, such as a user that does not exist. For one, RuntimeException is too broad. For another, we would have to catch this exception for every thing that calls it. I think this is fine if the exception isn't too broad (what if a RuntimeException happens in the internal code?). Alternatively, the function can simply return an invalid value of some sort and log some handwritten error. Also, raising a RuntimeException and forgetting to catch it will result in the entire backend (or server) crashing, which will require a restart.

Currently I'm not too sure how we should "implement" logging; we seem to rely heavily on these exceptions crashing the app and dumping the traceback once the app (or rather thread) is killed. Adding log statements around wouldn't be too hard, but can quickly litter the log (though it's probably fine). Also logback uses rotation logging with a max file size. Ideally, I want to hold an entire days log, and deal with the filesize constraints later. Then dump/copy the log into some form of long(er) term storage.

This PR also renames our package structure. I'm not sure why we were doing com.example.package, which is likely taken from the springboot boilerplate code, instead of just doing com.package, since com.example wouldn't actually point to anything.

Heads up, if we don't want to use Lombok for annotation, it's also fine, but we should determine early on (technically, lombok is a hack around java, so some developers may be opposed to it. It may also cause incompatibility issues with certain IDEs). It wouldn't be too big of a deal (right now), as we can manually put in the below or define our own annotation:

Logger logger = LoggerFactory.getLogger(LoggingController.class);

dependencies, add basic file logging (logback based off of slf4j with
lombok automation)
@IanChong1 IanChong1 self-requested a review August 21, 2024 12:37
@IanChong1
Copy link
Collaborator

exceptions are most oftenly used for users to see, logging is for internal engineers which we would have to deal with clients. Therefore you would always need both

Copy link
Collaborator

@IanChong1 IanChong1 left a comment

Choose a reason for hiding this comment

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

seems like you just change some file names right?

@stxue1
Copy link
Collaborator Author

stxue1 commented Aug 21, 2024

seems like you just change some file names right?

Filenames themselves don't change, I drop the example part of the path as there isn't much reason to include it.

exceptions are most oftenly used for users to see, logging is for internal engineers which we would have to deal with clients. Therefore you would always need both

Spring boot handles exceptions differently compared to other languages. They apparently have dedicated exceptions for what we're trying to do: https://docs.spring.io/spring-framework/docs/3.2.x/spring-framework-reference/html/mvc.html#mvc-ann-rest-spring-mvc-exceptions. We should use these types of exceptions instead of RuntimeException; RuntimeException implies error code 500 which indicates a server side error so it should be used sparingly. The way logging works right now is if an exception is encountered, a traceback is printed into the log. Adding log statements is fine and normal, the main thing is I don't have much documentation to work off of so it will take a while.

@anlhu
Copy link
Collaborator

anlhu commented Aug 28, 2024

Why were we ever throwing runtime exceptions? We should use ResponseEntity to send a response with a failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants