Conversation
application/src/main/java/org/togetherjava/tjbot/features/system/BotCore.java
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/system/BotCore.java
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/system/BotCore.java
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/system/BotCore.java
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/system/BotCore.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/analytics/AnalyticsService.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/analytics/AnalyticsService.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/system/BotCore.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/system/BotCore.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,13 @@ | |||
| CREATE TABLE command_usage | |||
There was a problem hiding this comment.
rename table to metrics or metric_events
There was a problem hiding this comment.
since u called it Metrics please also call the table metrics or metric_events, not analytics, thanks.
application/src/main/resources/db/V16__Add_Analytics_System.sql
Outdated
Show resolved
Hide resolved
application/src/main/resources/db/V16__Add_Analytics_System.sql
Outdated
Show resolved
Hide resolved
firasrg
force-pushed
the
firasrg/analytics
branch
from
f56996b to
25a4407
Compare
| error_message TEXT | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| event TEXT NOT NULL, | ||
| happened_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP |
There was a problem hiding this comment.
please remove the default, happened_at is mandatory to set, it shouldnt be defaulted.
There was a problem hiding this comment.
I don't see why not use DEFAULT CURRENT_TIMESTAMP here; this can be helpful sometimes, like to avoid repetitive code and bypass the necessity to instantiate Instant object from the Java code
There was a problem hiding this comment.
Because the event never happened at the moment of insertion into the DB but at the moment the caller of the count method says.
If thr default timestamp would be used it would always be incorrect. Remember that this process is async. Saving the event into the DB could happen ten minutes after count was called.
| * @param event the event to save | ||
| * @param moment the moment when the event is dispatched | ||
| */ | ||
| private void persist(String event, Instant moment) { |
There was a problem hiding this comment.
the name isnt ideal since this method might do more than just persisting. thats also an implementation-detail that might change.
rename it to processEvent instead.
There was a problem hiding this comment.
in fact, I found it not ideal too to put processEvent as a name, since the event is being processed in count(), and the role of this method is only about save/persist in db
There was a problem hiding this comment.
Your understanding of the methods role is incorrect. the count method doesnt do any processing. it merely forwards the call immediately to the service where it will be processed async to not block the caller.
| Instant moment = Instant.now(); | ||
| service.submit(() -> persist(event, moment)); | ||
|
|
||
| logger.debug("Event {} new record saved successfully", event); |
There was a problem hiding this comment.
that log is misleading. the call is async so at this point nothing might have happened yet. remove that log
There was a problem hiding this comment.
oh that's right, mb
| * @param event the event to save | ||
| * @param moment the moment when the event is dispatched | ||
| */ | ||
| private void persist(String event, Instant moment) { |
| * @param moment the moment when the event is dispatched | ||
| */ | ||
| private void persist(String event, Instant moment) { | ||
| logger.debug("Persisting event: {}, at {}", event, moment); |
There was a problem hiding this comment.
not sure why ur adding all these debug logs everywhere, we already have debug-level logs on the database side if needed
There was a problem hiding this comment.
it's for debugging in java code
| .setEvent(event) | ||
| .setHappenedAt(moment) | ||
| .insert()); | ||
| logger.debug("Event {} persisted successfully", event); |
|
3 participants
Early implementation of basic analytics for TJ Bot
What’s done so far:
AnalyticsServicefor analytics Logic and persistence in Database (V16 DB Migration included);/pingslash command use savedUPDATE 06/03/2025
Following @Zabuzard CR, the analytics feature is no more specified for slash commands. Now, it's open to track any other sort of events!
Demo: the new table structure
