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

jump to end of reports on init, better handle missing next_token #360

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jesopo
Copy link
Contributor

@jesopo jesopo commented Aug 19, 2022

in draft because this will still iterate the first page of reports when we first call the endpoint, even though we want to skip to the end

@jesopo
Copy link
Contributor Author

jesopo commented Aug 19, 2022

while i'm here, i should probably add a test for report polling now that config isn't static

* on the next endpoint call. If not, we're on the last page,
* which means we want to skip to the end of this page.
*/
from = response.next_token ?? this.from + response.event_reports.length;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.from + response.event_reports.length

isn't ideal because the from token is meant to be opaque, but i don't see a better way to do this. i've been in discussions with synapse devs to talk about how this endpoint could be better

* the end of available reports, so we'll only consider reports
* that happened after we supported report polling.
*/
from = response.total;
Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds fishy. Isn't from supposed to be an opaque identifier?

await this.mjolnir.logMessage(LogLevel.ERROR, "getAbuseReports", `failed to update progress: ${ex}`);
}
let from;
if (this.from === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that we want to do something to avoid displaying tens of thousands of old reports the first time we start the Report Poller on the server.

I believe that we should rather:

  • Upon startup of the ReportPoller.
    1. Load from from Mjölnir's account data.
    2. If there is no from
      a. Fetch a cutout date from the configuration, or some default date if not specified (say August 1st 2022).
      b. Start background enumerating all reports until the cutout date, without displaying them.
      c. Use this to initialize from. This may also give us a first few abuse reports that will need to be returned the first time we call getAbuseReports.
      d. Store this value of from in Mjölnir's account data.
  • Whenever we call getAbuseReports.
    1. Wait until background enumeration is complete before returning any new reports.
    2. By definition, we have a from (which may be null if the server has never encountered any abuse report).
    3. Proceed as usual.
    4. Whenever we update from, store it in Mjölnir's account data.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants