SAK-50846 Sitestats successive sitestats calculated incorrectly #13230

st-manu · 2025-01-23T16:20:53Z

https://sakaiproject.atlassian.net/browse/SAK-50846

Conversion: sakaiproject/sakai-reference#263

If a user is connected when the SiteStats Event Aggregator job is run, the user's previously closed session is merged with the active session.

real time presence
x-----x | x-------|----x
calculation
x--------------------------x

ottenhoff · 2025-01-23T16:54:51Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/PresenceConsolidation.java

@@ -54,6 +58,7 @@ public boolean add(@NonNull Presence presence) {

        if (records.contains(additialRecord)) {
            // Record is already present in list, leave unchanged
+            log.debug("Record already present: " + additialRecord);


"Concatenation is expensive, so you want it to happen only when needed. By using {}, slf4j performs the concatenation only if the trace is needed. In production, you may configure the log level to INFO, thus ignoring all debug traces."

Good point Sam! Most of the time debug is disabled.

I took the opportunity to update the rest of the concatenations in the log.debug statements of the modified files.

ottenhoff

this passes the tests locally for me.

just need to fix lombok, slf4j log concatenation.

thanks for the patch

ottenhoff · 2025-01-23T16:57:16Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/SitePresenceRecord.java

+    public void setOriginallyEnding(boolean originallyEnding) {
+        this.originallyEnding = originallyEnding;
+    }
+


the use of lombok @Data means these getters and setters are unnecessary

Yes, you can remove the getters, setters. I would change to Boolean. Though, you will need to replace the use of isOriginallyEnding with `getOriginallyEnding.

stetsche

Great work Manu! I left some comments for minor improvements.

stetsche · 2025-01-23T16:32:08Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/PresenceConsolidation.java


 import lombok.NonNull;

 public class PresenceConsolidation {


+    private static final Logger log = LoggerFactory.getLogger(PresenceConsolidation.class);


Use @Slf4j instead

stetsche · 2025-01-23T16:36:21Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/StatsUpdateManagerImpl.java

+
+			log.debug("currentOpenSessions: {}", currentOpenSessions);
+
+			if (!allUserSitePresences.isEmpty() && allUserSitePresences.get(0).isBeginning() && savedOpenSessions == 0) {


Is it safe to look just at the first entry, should we check if there is any beginning presence?

actually, that check isn't necessary; I removed it. if there were no open sessions (savedOpenSessions == 0), with the current logic, we can disregard the saved login start (savedBegin) since we are no longer dealing with overlapping sessions.

stetsche · 2025-01-23T16:40:43Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/StatsUpdateManagerImpl.java

@@ -1669,7 +1735,9 @@ private String parseSiteId(Event e){
 				if(contextId != null && contextId.startsWith(sitePrefix)) {
 					contextId = contextId.substring(sitePrefix.length());
 				}
-				log.debug("Context read from Event.getContext() for event: " + eventId + " - context: " + contextId);
+				if (eventId == "pres.end" || eventId == "pres.begin") {


Should we keep this condition or was it just for your testing?

exactly, I removed the condition, thnks

stetsche · 2025-01-24T08:27:55Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/PresenceConsolidation.java

@@ -54,6 +58,7 @@ public boolean add(@NonNull Presence presence) {

        if (records.contains(additialRecord)) {
            // Record is already present in list, leave unchanged
+            log.debug("Record already present: " + additialRecord);


Suggested change

log.debug("Record already present: " + additialRecord);

log.debug("Record already present: {}", additialRecord);

Here an example. Please apply this to the other debug logs as well @st-manu

stetsche · 2025-01-24T08:29:40Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/PresenceConsolidation.java

@@ -54,6 +58,7 @@ public boolean add(@NonNull Presence presence) {

        if (records.contains(additialRecord)) {
            // Record is already present in list, leave unchanged
+            log.debug("Record already present: " + additialRecord);


Good point Sam! Most of the time debug is disabled.

stetsche · 2025-01-24T08:32:45Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/SitePresenceRecord.java

+    public void setOriginallyEnding(boolean originallyEnding) {
+        this.originallyEnding = originallyEnding;
+    }
+


Yes, you can remove the getters, setters. I would change to Boolean. Though, you will need to replace the use of isOriginallyEnding with `getOriginallyEnding.

stetsche · 2025-01-24T08:42:14Z

Please also do a rebase on master

st-manu · 2025-01-27T10:33:52Z

thank you for your comments, @stetsche and @ottenhoff !

stetsche

Just found a little error. Else looking good

stetsche · 2025-01-27T16:49:59Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/StatsUpdateManagerImpl.java

@@ -1669,9 +1735,9 @@ private String parseSiteId(Event e){
 				if(contextId != null && contextId.startsWith(sitePrefix)) {
 					contextId = contextId.substring(sitePrefix.length());
 				}
-				log.debug("Context read from Event.getContext() for event: " + eventId + " - context: " + contextId);
+				log.debug("Context read from Event.getContext() for event: {} - context: ", eventId, contextId);


Missing placeholder

Suggested change

log.debug("Context read from Event.getContext() for event: {} - context: ", eventId, contextId);

log.debug("Context read from Event.getContext() for event: {} - context: {}", eventId, contextId);

ern · 2025-01-27T17:17:13Z

sitestats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/StatsUpdateManagerImpl.java

+				log.debug("k{} savedBegin: {}", key, savedBegin);
+				log.debug("k{} firstBeginningPresenceBegin: {}", key, firstBeginningPresenceBegin);
+				log.debug("k{} lastStart: {}", key, lastStart);


these debug statements should probably be consolidated

st-manu · 2025-01-28T09:09:30Z

@ern @stetsche changes applied ✅

stetsche

Looking good, thanks @st-manu! Could you do a rebase to be current with master please?

ottenhoff reviewed Jan 23, 2025

View reviewed changes

ottenhoff requested changes Jan 23, 2025

View reviewed changes

stetsche requested changes Jan 24, 2025

View reviewed changes

st-manu force-pushed the SAK-50846-fix branch 2 times, most recently from e762754 to 3c8fb8d Compare January 27, 2025 10:30

st-manu force-pushed the SAK-50846-fix branch from 3c8fb8d to bec94e9 Compare January 27, 2025 10:39

stetsche requested changes Jan 27, 2025

View reviewed changes

ern reviewed Jan 27, 2025

View reviewed changes

ern changed the title ~~SAK-50846 Sitestats: Successive sitestats calculated incorrectly~~ SAK-50846 Sitestats successive sitestats calculated incorrectly Jan 28, 2025

st-manu force-pushed the SAK-50846-fix branch from bec94e9 to 066220e Compare January 28, 2025 09:07

stetsche approved these changes Jan 28, 2025

View reviewed changes

st-manu added 2 commits January 29, 2025 08:29

SAK-50846 Sitestats: Successive sitestats calculated incorrectly

d02f14f

SAK-50846 Sitestats: Update unit tests

53b368b

st-manu force-pushed the SAK-50846-fix branch from 066220e to 53b368b Compare January 29, 2025 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAK-50846 Sitestats successive sitestats calculated incorrectly #13230

SAK-50846 Sitestats successive sitestats calculated incorrectly #13230

st-manu commented Jan 23, 2025 •

edited by stetsche

Loading

ottenhoff Jan 23, 2025

stetsche Jan 24, 2025

st-manu Jan 27, 2025

ottenhoff left a comment

ottenhoff Jan 23, 2025

stetsche Jan 24, 2025

stetsche left a comment

stetsche Jan 23, 2025

stetsche Jan 23, 2025

st-manu Jan 27, 2025

stetsche Jan 23, 2025

st-manu Jan 27, 2025

stetsche Jan 24, 2025

stetsche Jan 24, 2025

stetsche Jan 24, 2025

stetsche Jan 24, 2025

stetsche commented Jan 24, 2025

st-manu commented Jan 27, 2025

stetsche left a comment

stetsche Jan 27, 2025

ern Jan 27, 2025

st-manu commented Jan 28, 2025

stetsche left a comment


		log.debug("currentOpenSessions: {}", currentOpenSessions);

		if (!allUserSitePresences.isEmpty() && allUserSitePresences.get(0).isBeginning() && savedOpenSessions == 0) {

	log.debug("Record already present: " + additialRecord);
	log.debug("Record already present: {}", additialRecord);

	log.debug("Context read from Event.getContext() for event: {} - context: ", eventId, contextId);
	log.debug("Context read from Event.getContext() for event: {} - context: {}", eventId, contextId);

SAK-50846 Sitestats successive sitestats calculated incorrectly #13230

Are you sure you want to change the base?

SAK-50846 Sitestats successive sitestats calculated incorrectly #13230

Conversation

st-manu commented Jan 23, 2025 • edited by stetsche Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ottenhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stetsche 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stetsche commented Jan 24, 2025

st-manu commented Jan 27, 2025

stetsche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st-manu commented Jan 28, 2025

stetsche left a comment

Choose a reason for hiding this comment

st-manu commented Jan 23, 2025 •

edited by stetsche

Loading