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.

Sign up for GitHub

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

Jump to bottom

fix: remove link statistics and ga served events #243

Merged
merged 5 commits into from
Jul 2, 2020
Merged

fix: remove link statistics and ga served events #243

merged 5 commits into from
Jul 2, 2020

Conversation

kylerwsm
Copy link
Contributor

@kylerwsm kylerwsm commented Jul 1, 2020

Problem

  • Remove link statistics logging code.
  • Improvements from @LoneRifle earlier PR here

Solution

  • Move GA page view logging to after page redirect
  • Remove link statistics logging, which as of right now is problematic
  • Remove GA served events, which can lighten the network load on our servers

LoneRifle and others added 4 commits July 1, 2020 17:37
@LoneRifle @kylerwsm
refactor(stats): rework sequelize queries
a3e9256 
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
@LoneRifle @kylerwsm
perf(db): allow db pool max to be controlled, default 20
e27a0f0
@kylerwsm
perf(stats): remove ga logging for served event
c05afde
@kylerwsm
fix(stats): remove link stats logging except for total clicks
a854caf
@kylerwsm kylerwsm marked this pull request as ready for review July 1, 2020 13:06
LoneRifle
LoneRifle approved these changes Jul 2, 2020
View reviewed changes
Copy link
Contributor

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

lgtm. I'm unsure whether removing userAgent would be appropriate given that we are probably adding it back later on, but you don't have to make the change now.

@liangyuanruo
Copy link
Contributor

liangyuanruo commented Jul 2, 2020 •
edited
Loading

hold off from merging first.

@kylerwsm kylerwsm requested review from yong-jie and JasonChong96 July 2, 2020 05:54
liangyuanruo
liangyuanruo reviewed Jul 2, 2020
View reviewed changes
src/server/config.ts Outdated Show resolved Hide resolved
JasonChong96
JasonChong96 approved these changes Jul 2, 2020
View reviewed changes
Copy link
Contributor

@JasonChong96 JasonChong96 left a comment

Choose a reason for hiding this comment

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

lgtm

@kylerwsm @liangyuanruo
perf(stats): update default db pool size
1874267 
Co-authored-by: Yuan Ruo <liangyuanruo@gmail.com>
yong-jie
yong-jie approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@kylerwsm kylerwsm merged commit 454dbab into develop Jul 2, 2020
@kylerwsm kylerwsm deleted the rm-stats branch July 2, 2020 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liangyuanruo liangyuanruo liangyuanruo left review comments

@JasonChong96 JasonChong96 JasonChong96 approved these changes

@yong-jie yong-jie yong-jie approved these changes

@LoneRifle LoneRifle LoneRifle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kylerwsm @liangyuanruo @LoneRifle @yong-jie @JasonChong96