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.

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

do not update token usage in tracking requests #16925

Merged
merged 3 commits into from Dec 16, 2020
Merged

do not update token usage in tracking requests #16925

merged 3 commits into from Dec 16, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Dec 9, 2020

Description:

Seems updating the usage timestamp of a token might cause issues when using log import.
Not sure if we need to update the usage at all in this case. If so we maybe could also update it every 10 minutes only or so, but that would at least require an additional read request to check that.

fixes #16924

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Dec 9, 2020
@tsteur
Copy link
Member

tsteur commented Dec 9, 2020

Seems this doesn't fix the issue? Also it shouldn't really be a problem I think as when using the API concurrently we would see these issues as well maybe? Generally could change the where though like

UPDATE 'matomo_user_token_auth' SET 'last_used' = ? WHERE 'idusertokenauth' = ? and last_used < ?

It likely wouldn't make a difference though . We basically only want to update it when time() is higher. When value doesn't change then mysql should be smart enough though.

@sgiehl
Copy link
Member Author

sgiehl commented Dec 11, 2020

Maybe that's only one part of the issue. But firing thousands of updates on the same row isn't good in any case.
Will have a closer look at the code again later. Maybe there's any easy way to update it like only every 10 minutes or so

@tsteur
Copy link
Member

tsteur commented Dec 12, 2020

Be great to indeed only update it like every 10 min (in tracker and in general). Probably can't use general tracker cache as it would invalidate the cache every 10min when we use a much longer cache time on cloud.(1hr+)

@sgiehl
Copy link
Member Author

sgiehl commented Dec 14, 2020

@tsteur updated the PR. There will now always be an select to check if the last date for the token is older than 10 minutes. Not sure if that might make problems if multiple requests are executed at the same time

@tsteur tsteur merged commit b8faca7 into 4.x-dev Dec 16, 2020
@tsteur tsteur added this to the 4.1.0 milestone Dec 16, 2020
@sgiehl sgiehl deleted the tokenusage branch January 27, 2021 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tons of "General error: 1205" in php error log while importing via the python log importer
2 participants