-
Notifications
You must be signed in to change notification settings - Fork 539
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
Add command to check locales completion rates and send emails about it #23153
Conversation
src/olympia/amo/management/commands/check_locales_completion_rate.py
Outdated
Show resolved
Hide resolved
The following locales are above threshold and not yet enabled: | ||
- {{ locales_above_threshold }} | ||
{% endif %} | ||
{% if not locales_below_threshold and not locales_above_threshold and not locales_to_keep_despite_being_below_threshold %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just not send the email in this case or? What is the argument for regularly sending an email with no information in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only going to be sent monthly, so it's not going to be super spammy.
I wanted to still send it in this case to let people know everything is working properly. It's a cheap built-in monitoring system :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small-medium sized nits. I think I would strongly recommend the:
- task for core logic to benefit from standard retry (we should start making that the default way to implement async logic that has external dependencies.
- set intersection for duplicate/conflicting results
- single request for all the data and formatting in the fetch method (easier to read and test and fewer things can go wrong)
self.stdout.write(f'Calling pontoon with {q}') | ||
response = requests.get(self.PONTOON_API, {'query': q}) | ||
response.raise_for_status() | ||
data = response.json().get('data', {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Graphql apis generally return errors in the response and usually response 200 even with errors.. we might need to check if pontoon might be returning an "error" object in the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had 400 for incorrect fields. For instance, this 400s:
curl -gi 'https://pontoon.mozilla.org/graphql?query={projecterrorpleaseignore(slug:"amo-frontend"){name,localizations{locale{code,name}totalStrings,approvedStrings}}}'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if it returns an error property in the response. GraphQL apis are weird and can fail both with status or property. Depends on how pontoon implemented it but might be worth double checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does return errors
but it would raise before we parse since it returns a proper error code.
src/olympia/amo/management/commands/check_locales_completion_rate.py
Outdated
Show resolved
Hide resolved
src/olympia/amo/management/commands/check_locales_completion_rate.py
Outdated
Show resolved
Hide resolved
return data | ||
|
||
def get_completion(self, locale_data): | ||
return round(locale_data['approvedStrings'] / locale_data['totalStrings'] * 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if totalStrings
is ever zero, this could raise. might want to have a quick check for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And do what though ? If pontoon is reporting a 0
here I'm okay with raising: this should be reported in Sentry, it's unexpected behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is 0 you could just return 0. That would indicate there are no translations for a locale right? Since this is a job that runs automatically and regularly might be good to avoid typical cases where it could raise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer raising, because it's not expected behavior. If we return 0
then it will impact the output and if it's a global bug affecting multiple locales we'd never know which locales should legitimately be removed or which just had a bug
src/olympia/amo/management/commands/check_locales_completion_rate.py
Outdated
Show resolved
Hide resolved
src/olympia/amo/management/commands/check_locales_completion_rate.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works

UX it's a bit hard to read through the messages as they are unstructured.. probably not worth it but having headers and organized messages would probably read more smoothely.
Locales to add
===========
✅ Hindi (India) [hi-IN] is above threshold (44%)
... more locales that are not currently enabled but above the threshold
Locales to remove
===========
❌ Irish [ga-IE] is below threshold (30%)
... other locales that are below threshold in at least one project or missing from at least one project
Locales to keep
===========
✅ Interlingua [ia] is above threshold (100%) in amo
locales that are above threshold in both projects and already enabled
That's pretty close to your email template... wondering if instead of the custom logs, you just render and print the email template 🤷
The standard output is really only useful for debugging or if we want to dig deeper in the results - it's more verbose, since it includes completion per project, something the people receiving the email won't care about. |
Fixes: mozilla/addons#15175
Context
We're going to use this new command to check how complete translations are in AMO (starting with a fairly low threshold of 40%) and warn us about anything weird, using pontoon API.
This will be added to AMO cronjobs (running from AMO infra gets us an email server and actual prod settings) by SRE, to be run monthly.
Testing
Run
manage.py check_locales_completion_rate
to try it locally, checkout out the standard output as well as the fake email to see what it has generated - depends on pontoon API response.Check included tests for more responses with forced fake content.