Write DomainMetrics and add FF to switch reads from calculated properties #35522
Conversation
dimagimon
added
reindex/migration
nospame
force-pushed
the
em/write-domain-metrics
branch
from
075ab84 to
42eafa5
Compare
There was a problem hiding this comment.
I know this is a pretty minor migration, but I think the general rule of thumb to apply migrations in a separate PR to be able to easily rollback the code that depends on it if necessary is still valid here. Granted, it looks like you made the migration reversible so I'm not sure how much it really matters, but I'd lean towards moving out the migration since it is pretty straightforward anyway.
Overall this looks great!
| metrics_dict['domain'] = domain_obj.name | ||
| metrics_dict['last_modified'] = datetime.now(tz=timezone.utc) | ||
|
|
||
| # these are properties on the Django model, so don't try to write them |
There was a problem hiding this comment.
nit: could be a bit more explicit to say these are "calculated fields on the Django model" or something like that. I recognize that it is a tough situation since we have referred to the DomainMetrics as calculated properties, but hopefully going forward we can limit where we use "calculated properties" to avoid conflicting with the conventional definition of that term.
There was a problem hiding this comment.
What would you think of "these are model properties on DomainMetrics"? "Calculated fields" gets the point across just as well but it doesn't feel as technically correct to me.
There was a problem hiding this comment.
Oh interesting. In my mind, calculated (or computed) is technically correct when referring to these properties/fields of a model class, since they aren't stored in the table but instead calculated/computed after fetching the object from the database.
There was a problem hiding this comment.
That's fair actually, I guess it maybe depends whether you're looking at it from the database's perspective or the code's -- whether it's a "calculated" field versus a regular field, or if it's a "property" versus a regular model method.
| if metrics: | ||
| active_users_by_domain[domain] = metrics['active_mobile_workers'] | ||
|
|
||
| datadog_report_user_stats('commcare.active_mobile_workers.count', active_users_by_domain) |
There was a problem hiding this comment.
Just wondering out loud, but maybe it would be nice to track more of these metrics in datadog.
nospame
changed the base branch from
master
to
em/domain-metrics-model-changes
dimagimon
removed
the
reindex/migration
nospame
force-pushed
the
em/write-domain-metrics
branch
from
6c5d5b4 to
1f9cff1
Compare
nospame
changed the base branch from
em/domain-metrics-model-changes
to
master
Co-authored-by: Graham Herceg <[email protected]>
dimagimon
added
the
reindex/migration
dimagimon
removed
the
reindex/migration
Technical Summary
SAAS-16283
Writes "calculated properties" currently stored on the
DomainElasticsearch doc to the newDomainMetricsSQL model and adds a feature flag so that reads can be switched from the ES doc to the SQL model once populated. This is part 2 of 3 for moving calculated properties into SQL, following #35415. This should be reviewable by commit.Force-pushed to pull out migration commits #35574
Feature Flag
Adds the slightly wordy
CALCULATED_PROPERTIES_FROM_DOMAIN_METRICSfeature flag, which can be used to switch calculated properties reads over to the SQL model for a single domain, to toggle on for all domains.Safety Assurance
Safety story
This doesn't affect existing data or the write process for calculated properties on the ES doc. Reading from the new model is controlled by a feature flag, so if there are any issues after switching over, it can easily be switched back.
Automated test coverage
Adds automated tests for identifying the correct project spaces to have their
DomainMetricsupdated. These are based on existing tests that look to see the right domains will get their calculated properties updated.QA Plan
I can see an argument for QA, but not planning it at this time. I think dev testing on staging will suffice.
Migrations
No migrations here, but must follow migration applied in #35574
Rollback instructions
Labels & Review