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

[ENG-4438] Add OOPSpam and Akismet metrics to spam report #10783

Conversation

uditijmehta
Copy link
Contributor

@uditijmehta uditijmehta commented Oct 21, 2024

Purpose

Add OOPSpam and Akismet metrics to track flagged and hammed content

Changes

  • SpamCountReporter: Added OOPSpam and Akismet spam/ham metrics for nodes, registrations, preprints, and users.
  • SpamSummaryReport: Added new fields for OOPSpam and Akismet flagged/hammed counts.
  • OOPSpamClient & AkismetClient: Added methods to get flagged and hammed content counts.

Ticket

(https://openscience.atlassian.net/browse/ENG-4438)

Deployment Note: Because the metric report has changed, it will be necessary to run sync_metrics to apply these changes to the updated spam metrics report.

Copy link
Member

@mfraezz mfraezz left a comment

Choose a reason for hiding this comment

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

Quick Drive-By CR. One functional issue that needs fixing in a couple places, and some smaller things preventing the build from passing.

Please also add a Deployment Note in the PR description that, because the metric report is changing, sync_metrics will need to be ran

osf/external/askismet/client.py Outdated Show resolved Hide resolved
osf_tests/metrics/test_spam_count_reporter.py Outdated Show resolved Hide resolved
osf_tests/metrics/test_spam_count_reporter.py Outdated Show resolved Hide resolved
@uditijmehta uditijmehta force-pushed the add-oopspam-akismet-metrics branch 5 times, most recently from e93994a to 5d9f924 Compare October 28, 2024 19:30
Copy link
Member

@mfraezz mfraezz left a comment

Choose a reason for hiding this comment

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

The code/tests itself LGTM, however in reviewing the ticket and considering the request, two relevant considerations came up:

  1. Description, comments on this ticket indicate that it should be in a private metrics Report, but it isn't clear whether this information should be in a new private Report, or if it should be added to the SpamCountReporter (as it currently is) and that report should be made private.
    Recommendation: seek clarity from Product regarding this. In either case, some additional work will be required
  2. The spirit of this request is in seeking clarity on true-vs-false positive rates of automatic spam classification, however this PR only address Nodes. Preprints are another, perhaps larger category of spam, and we may want to extend this functionality to Preprints as well.
    Recommendation: Confirm if that is desired by Product, and if so add a category param to the get_*_count fn's to keep things DRY, and conditionally use PreprintLog, preprint__spam_data__<etc> instead. Extend report, tests as appropriate.

Pass complete :octocat:

@uditijmehta uditijmehta force-pushed the add-oopspam-akismet-metrics branch 5 times, most recently from 39f9526 to f862984 Compare November 4, 2024 20:29
@uditijmehta
Copy link
Contributor Author

@mfraezz I checked in with Product, and based on that, I made the updates—set up a new private metrics report, included preprints, and added the category parameter.

@mfraezz mfraezz changed the base branch from feature/b-and-i-24-20 to feature/b-and-i-24-22 November 5, 2024 15:43
Copy link
Member

@mfraezz mfraezz left a comment

Choose a reason for hiding this comment

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

Aside from one minor comment, code here looks good. 🍔

However, in order for the new report to run on the monthly cadence, it will also need to be added to the AllMonthlyReporters Enum in osf/metrics/reporters/__init__

Should be ready to merge after that. Pass complete :octocat:

There may be a follow-up ticket created to make this new report available, with adequate permissions restrictions, in the metrics API. However, I expect that to be enough work to merit its own ticket, so can be deferred for now.

osf/external/askismet/client.py Show resolved Hide resolved
@mfraezz mfraezz merged commit a1770a6 into CenterForOpenScience:feature/b-and-i-24-22 Nov 5, 2024
6 checks passed
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