[#10950] Add logging functionality to backend #10984

halfwhole · 2021-02-26T14:35:00Z

Adds the ability to create and search for feedback session logs. When a student accesses the feedback session page, it'll create a log with the student's details, which might look something like this in the Logs Explorer:

The methods in LoggingService will throw an exception if they fail. In the case of the student failing to create a feedback session log entry, it will also log a severe warning for admin purposes
Still need to fix the return type of getLogEntries, can probably only do so after we finalise the API types

teammates-bot · 2021-02-26T14:35:11Z

Hi @halfwhole, these parts of your pull request do not appear to follow our contributing guidelines:

PR Title
- Issue Reference (#<issue-number>) missing.

src/main/java/teammates/logic/core/LoggingLogic.java

t-cheepeng · 2021-02-27T04:17:16Z

src/main/java/teammates/logic/core/LoggingLogic.java

+        }
+        String payload = "Feedback session log: courseId=" + courseId + " email=" + email;
+        String type = isAccess ? LOG_TYPE_ACCESS : LOG_TYPE_SUBMIT;
+        LogEntry entry = LogEntry.newBuilder(StringPayload.of(payload))


Actually do we need the payload since we have the labels already?

I'm also wondering what the payload should be! We don't strictly need it, but it seems a bit strange to have logs with no description to them in the console. Since we shouldn't be exceeding the log storage limit, it might be helpful to have some form of payload

t-cheepeng · 2021-02-27T04:26:09Z

The main methods in LoggingService currently log a severe warning upon failure. Is this OK, or should it throw something like a RuntimeException instead?

For actual logging, the severe log should be okay. But for filtering/retrieval of logs, probably best to return Exception with error message to frontend for display

ChooJeremy · 2021-02-28T16:02:48Z

The main methods in LoggingService currently log a severe warning upon failure. Is this OK, or should it throw something like a RuntimeException instead?

For actual logging, the severe log should be okay. But for filtering/retrieval of logs, probably best to return Exception with error message to frontend for display

Hmm, one of the use cases for this is to allow instructors to verify student's claims. If this occurs (logging fails), does this then mean the student may actually be correct and the instructor thinks that the student is bluffing? This seems dangerous. The instructor could ask the admin (Prof. in this case) but it will likely end up being very bothersome if instructors have to ask admin to verify each time they can't find a corresponding log entry.

t-cheepeng · 2021-03-02T02:28:16Z

If this occurs (logging fails), does this then mean the student may actually be correct and the instructor thinks that the student is bluffing?

If the logging fails and we throw an error to the frontend, then the students can still continue submitting the session but we display a message/toast stating that the student is not being tracked for their actions and any dispute on submission times are not guaranteed. or we could disable the submission but that seems overkill and inconvenient

@damithc What should the expected experience for students be if the logging fails on the backend and we are not able to track the student?

damithc · 2021-03-02T02:54:03Z

@damithc What should the expected experience for students be if the logging fails on the backend and we are not able to track the student?

I think it is OK to save but caution the user as disputes usually happens for cases of non-saving. But we (i.e., admin) want to know if this happens at all.

A more important question is, how to notify admin if logging fails? Right now, logging is used to notify admin of failures (i.e., any error logs are emailed to the admin).

madanalogy · 2021-03-02T04:13:41Z

A more important question is, how to notify admin if logging fails? Right now, logging is used to notify admin of failures (i.e., any error logs are emailed to the admin).

This is a problem we don't really have a solution to as yet. Might be worthwhile to start an issue on it. Relevant discussion: #10930 (comment)

halfwhole · 2021-03-04T12:02:39Z

In view of the discussion above, if we fail to create a feedback session log using the Cloud Logging API, would it be good to both throw an error to the frontend (for the student's sake), and use the standard error logging format with log.severe to register the error (which might not fail even if Cloud Logging fails)? Since we don't have a solution to standard error logging failing as of yet, this might be best we can do for now?

madanalogy · 2021-03-04T13:40:42Z

I like the idea of returning an error to the frontend. The user can try refreshing the page or contact the admin. I don't think additional error logging would be helpful because like you pointed out, it wouldn't go through anyway.

src/main/java/teammates/ui/webapi/CreateFeedbackSessionLogAction.java

madanalogy

See comments below. I think it might be worthwhile to at least get the test skeletons up for the logic and webapi components. Can put a TODO in them and handle the actual writing of tests in a separate PR

madanalogy · 2021-03-11T14:33:56Z

src/main/java/teammates/logic/core/LoggingLogic.java

+/**
+ * Handles the logic related to logging.
+ */
+public final class LoggingLogic {


I think LogsLogic might be more appropriate, as it's the logic for managing logs (the noun). Just an opinion though so willing to hear what others might think

madanalogy · 2021-03-11T14:35:13Z

src/main/java/teammates/common/exception/LoggingServiceException.java

+ * Exception thrown when the logging service fails to create or retrieve logs.
+ */
+@SuppressWarnings("serial")
+public class LoggingServiceException extends Exception {


LogsServiceException though similar to other comment

madanalogy · 2021-03-11T14:37:00Z

src/main/java/teammates/logic/core/LoggingLogic.java

+    /**
+     * Gets the feedback session logs as filtered by the given parameters.
+     */
+    // TODO: decide on a return data format; this will likely be determined by our API


I think it might be worthwhile to decide on the data format first, considering that the logic might change because of data needs. I'd rather put this PR on hold until the data format / API is settled

rrtheonlyone · 2021-03-14T15:57:31Z

@halfwhole let us know when this PR is ready to be merged in - I think we can do a more thorough review once we have a MVP up

halfwhole · 2021-03-15T13:39:06Z

PR is ready to be merged/reviewed -- it's been integrated with the changes in #10978 to support both creating logs and retrieving logs

madanalogy

LGTM

rrtheonlyone

Alright took a look, progress is there 👍

However, it is definitely a little over-engineered at the moment. This is a simple feature and I feel it does not need to spam so many files.

I have put my comments below for discussion.

rrtheonlyone · 2021-03-16T14:14:01Z

src/main/java/teammates/logic/core/LogsService.java

+ *
+ * @see <a href="https://cloud.google.com/logging/docs">https://cloud.google.com/logging/docs</a>
+ */
+public class LogsService {


There is already a GoogleCloudLoggingService we can use from master - can merge with that

Yep, rebased this branch to take in these newer changes from master. Can work with those instead
Most parts have now been shifted to the newer GoogleCloudLoggingService and LogsProcessor files

rrtheonlyone · 2021-03-16T14:14:57Z

src/main/java/teammates/common/exception/LogsServiceException.java

+ * Exception thrown when the logs service fails to create or retrieve logs.
+ */
+@SuppressWarnings("serial")
+public class LogsServiceException extends Exception {


Why are we creating our own Exception for this?

Originally I had intended to use RuntimeException for this, but the linter complains when such raw exceptions are thrown. It also does not fit into any other exception class we have currently defined in teammates.common.exception, so I decided to create a new one here

rrtheonlyone · 2021-03-16T14:24:35Z

src/main/java/teammates/logic/core/LogsLogic.java

@@ -0,0 +1,108 @@
+package teammates.logic.core;


I don't think there is a need for this file at all - just update the service class with the below methods

src/main/java/teammates/ui/webapi/CreateFeedbackSessionLogAction.java

rrtheonlyone · 2021-03-16T14:28:26Z

src/main/java/teammates/ui/webapi/CreateFeedbackSessionLogAction.java

+        }
+
+        try {
+            logic.createFeedbackSessionLog(courseId, studentEmail, fsName, fslType);


better to use LogsProcessor here instead of logic layer

src/main/java/teammates/ui/webapi/CreateFeedbackSessionLogAction.java

src/main/java/teammates/ui/webapi/GetFeedbackSessionLogsAction.java

wkurniawan07

I happen to notice this. Just leaving two minor observations, will not comment any further. No need to involve me for any subsequent review.

wkurniawan07 · 2021-03-17T03:54:34Z

src/main/java/teammates/logic/core/GoogleCloudLoggingService.java

+        return fsLogEntries;
+    }
+
+    private List<LogEntry> getLogEntries(LogSearchParams s) throws LogServiceException {


You can probably refactor the existing getRecentErrorLogs method to also make use of this.

Should this be done separately on the master branch instead? It can be done here too, but was a bit hesitant cos this branch is specifically for the audit logs feature

wkurniawan07 · 2021-03-17T03:55:02Z

src/main/java/teammates/logic/core/GoogleCloudLoggingService.java

+            for (LogEntry entry : entries.iterateAll()) {
+                logEntries.add(entry);
+            }
+            logging.close();


You should not need this. The fact that logging is wrapped within try-with-resources tells that this will be automatically done.

Thanks, removed

t-cheepeng

Let's add in

SESSION_LOGS(ResourceURIs.SESSION_LOGS); to ResourceEndpoints.java. Otherwise frontend will miss out on this endpoint

t-cheepeng · 2021-03-19T09:14:41Z

src/main/java/teammates/ui/constants/LogType.java

+    /**
+     * Returns the enum value of a log type given its label.
+     */
+    public static LogType valueOfLabel(String label) {


Just a question here:

If you pass in values like
"access", "submission",
this method returns
"FEEDBACK_SESSION_ACCESS", "FEEDBACK_SESSION_SUBMISSION"
so the latter strings get returned to the frontend, but the frontend currently just expects former. Is there any reason to have the valueOfLabel? Since the generateTypes command just generates

export enum LogType { FEEDBACK_SESSION_ACCESS = "access", FEEDBACK_SESSION_SUBMISSION = "submission", }

so the comparison is with the "access" and "submission"

valueOfLabel here should have no effect on the frontend, it's mainly for the backend to be able to convert a given string such as "access" to the appropriate LogType (such as in the constructor of FeedbackSessionLogEntryData)

Right. But in the logic of sending back the results, the constructor of FeedbackSessionLogEntryData is called so the logType field in that data becomes the name of the label instead of "access"/"submission". So when the response is sent back to frontend, "FEEDBACK_SESSION_ACCESSS" is sent instead of "access". Was thinking that we can just simply send "access" instead of the label name since frontend will(probably) use the "access". Avoids having to do double work of checking for the label instead

Would that mean that our API should return a string instead of a LogType (thus this file can be removed entirely)?
It seems useful though for LogType to exist as part of an interface that guarantees our log types are one of only several enumerated options (such as FEEDBACK_SESSION_ACCESS or FEEDBACK_SESSION_SUBMISSION), rather than a string that implies something more open-ended

t-cheepeng · 2021-03-19T09:23:19Z

src/web/services/log.service.ts

@@ -20,7 +20,7 @@ export class LogService {
    courseId: string,
    feedbackSessionName: string,
    studentEmail: string,
-    logType: LogTypes }): Observable<string> {
+    logType: LogType }): Observable<string> {
    const paramMap: Record<string, string> = {
      courseid: queryParams.courseId,
      fsname: queryParams.feedbackSessionName,


Since the endpoint was changed to post, the method for this method should change too

rrtheonlyone

LGTM 👍

Let's refactor getLogEntries and getRecentErrorLogs from the GoogleCloudLoggingService in a separate PR.

* Add logging functionality to backend * Make changes to feedback session log logic * Add non null for student email * Rename logging to logs and add test skeletons * Add ability to retrieve logs via API * Fix frontend naming * Shift audit log functionality to logging service * Remove closing of logger * Change frontend components

halfwhole changed the title ~~Add logging functionality to backend~~ [#10950] Add logging functionality to backend Feb 26, 2021

halfwhole changed the base branch from master to audit-logs February 26, 2021 14:37

halfwhole force-pushed the 10950-logging-backend-audit-logs branch from 6aff23b to eeea9c2 Compare February 26, 2021 14:39

halfwhole added the s.OnHold The issue/PR's validity has been put on hold pending some other event label Feb 26, 2021

halfwhole assigned AdithyaNarayan and t-cheepeng and unassigned AdithyaNarayan and t-cheepeng Feb 26, 2021

halfwhole requested review from t-cheepeng and AdithyaNarayan February 26, 2021 14:46

t-cheepeng reviewed Feb 27, 2021

View reviewed changes

t-cheepeng requested review from ChooJeremy and rrtheonlyone February 27, 2021 04:26

rrtheonlyone requested a review from madanalogy February 27, 2021 10:36

t-cheepeng added s.ToReview The PR is waiting for review(s) and removed s.OnHold The issue/PR's validity has been put on hold pending some other event labels Mar 3, 2021

madanalogy force-pushed the audit-logs branch from 6518a34 to b89e1bc Compare March 4, 2021 02:19

halfwhole force-pushed the 10950-logging-backend-audit-logs branch from eeea9c2 to f15d748 Compare March 4, 2021 11:50

halfwhole mentioned this pull request Mar 4, 2021

[#10950] Track student feedback session in frontend #11005

Merged

t-cheepeng reviewed Mar 6, 2021

View reviewed changes

src/main/java/teammates/ui/webapi/CreateFeedbackSessionLogAction.java Outdated Show resolved Hide resolved

halfwhole force-pushed the 10950-logging-backend-audit-logs branch from 6d6061d to f7df937 Compare March 10, 2021 16:16

madanalogy reviewed Mar 11, 2021

View reviewed changes

rrtheonlyone added the s.ToReview The PR is waiting for review(s) label Mar 14, 2021

halfwhole force-pushed the 10950-logging-backend-audit-logs branch from cea1666 to 0306f6d Compare March 15, 2021 09:20

madanalogy approved these changes Mar 16, 2021

View reviewed changes

rrtheonlyone requested changes Mar 16, 2021

View reviewed changes

rrtheonlyone added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Mar 16, 2021

halfwhole added 6 commits March 16, 2021 23:37

Add logging functionality to backend

ac391b4

Make changes to feedback session log logic

3f451ad

Add non null for student email

2ac6eb6

Rename logging to logs and add test skeletons

9655522

Add ability to retrieve logs via API

1dd0d57

Fix frontend naming

5082124

halfwhole force-pushed the 10950-logging-backend-audit-logs branch from 7d4ffa1 to 5082124 Compare March 16, 2021 18:20

Shift audit log functionality to logging service

ce74549

halfwhole requested a review from rrtheonlyone March 17, 2021 03:01

halfwhole added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 17, 2021

wkurniawan07 reviewed Mar 17, 2021

View reviewed changes

Remove closing of logger

cbe2c7d

t-cheepeng reviewed Mar 19, 2021

View reviewed changes

Change frontend components

73f9d78

rrtheonlyone approved these changes Mar 21, 2021

View reviewed changes

rrtheonlyone merged commit 5f97283 into TEAMMATES:audit-logs Mar 21, 2021

[#10950] Add logging functionality to backend #10984

[#10950] Add logging functionality to backend #10984

Conversation

halfwhole commented Feb 26, 2021 • edited

teammates-bot commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-cheepeng commented Feb 27, 2021

ChooJeremy commented Feb 28, 2021

t-cheepeng commented Mar 2, 2021

damithc commented Mar 2, 2021

madanalogy commented Mar 2, 2021

halfwhole commented Mar 4, 2021 • edited

madanalogy commented Mar 4, 2021

madanalogy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrtheonlyone commented Mar 14, 2021

halfwhole commented Mar 15, 2021 • edited

madanalogy left a comment

Choose a reason for hiding this comment

rrtheonlyone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkurniawan07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-cheepeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrtheonlyone left a comment

Choose a reason for hiding this comment

halfwhole commented Feb 26, 2021 •

edited

halfwhole commented Mar 4, 2021 •

edited

halfwhole commented Mar 15, 2021 •

edited