Link Statistics with SQL Server Functions #264
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
liangyuanruo
reviewed
Jul 7, 2020
liangyuanruo
reviewed
Jul 7, 2020
liangyuanruo
reviewed
Jul 7, 2020
liangyuanruo
requested changes
Jul 7, 2020
liangyuanruo
reviewed
Jul 7, 2020
| ON CONFLICT ("shortUrl", "date") | ||
| DO UPDATE SET "clicks" = "${clicksTable}"."clicks" + 1; | ||
| -- Update weekday clicks. | ||
| INSERT INTO "${weekdayTable}" ("shortUrl", "weekday", "hours", "clicks", "createdAt", "updatedAt") |
There was a problem hiding this comment.
what is your plan to convert and display data in GMT+8 to the user?
There was a problem hiding this comment.
From my previous testings, the timestamps the function is using from the database is already in GMT+8. And since the logged link statistics data strips all timezone information, there should not be a need to convert the timezone of the data to the user.
There was a problem hiding this comment.
i believe that's only because your localhost time is in GMT+8. the database in staging/production are in UTC.
select current_timestamp;
> 2020-07-14 04:38:38.918593+00There was a problem hiding this comment.
I have rebased, and added explicit timezones to the function
liangyuanruo
requested changes
Jul 14, 2020
LinkStatisticsRepository updates click stats by finding the instance, then incrementing. This may result in larger transactions than needed, given that the increment can be done in a single statement. Rework the implementation so that we update, and if not present, we insert. We will also do this in many small transactions. Attempts to implement using `Model.upsert()` were not successful, due to Sequelize creating temporary functions which fail to reference the target table. - interface - rework jsdoc, change return type to boolean - impl - use `Model.update()` and `Sequelize.literal`, if not found, `Model.insert()` or throw not found error - Drop the constraint of doing stats updates in a single txn
liangyuanruo
approved these changes
Jul 15, 2020
xming13
referenced
this pull request
in xming13/GoGovSG
Aug 21, 2020
* feat(link statistics) Use a single SQL function to increment clicks across the following tables: - devices_stats - daily_stats - weekday_stats - urls Transactions whedn updating clicks are implicit as operations are carried out within the scope of a function, which either succeed in their entirety or rolled back upon failure. Co-authored-by: LoneRifle <LoneRifle@users.noreply.github.com>
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.
Problem
The previous attempt to log link statistics is too computationally expensive.
We needed an alternative that is able to handle and thrive under high user traffic.
Solution
Rather than logging using Sequelize model instances, we turned to SQL server functions. This function will be initialised by the server when it starts, and will be called to increment the relevant statistics on redirects.
Additionally, @liangyuanruo and I have load tested this implementation with the following results: