Application Version Compliance Report

[jingcheng16]

Product Description

Frontend

Report

Technical Summary

Ticket: https://dimagi.atlassian.net/browse/SAAS-16311

Review by commit. ( The commit history is basically how I improved the performance step by step )

This PR is to add a new Application Version Compliance Tile to the Enterprise Console.
This tile will iterate through all domain in this enterprise account, in each domain, iterate through all mobile workers, for each mobile worker, iterate through all their last_build in last_builds, (last_builds include the last build of the application they installed), and see if the build is out of date, if it is, then append it to the report's row.

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

• This PR can be reverted after deploy with no further considerations

Labels & Review

• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[sentry-io]

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: corehq/apps/enterprise/enterprise.py

Function	Unhandled Issue
rows_for_domain	AttributeError: 'EnterpriseAppVersionComplianceReport' object has no attribute 'domain' corehq.apps.e... Event Count: 2

---

_{Did you find this useful? React with a 👍 or 👎}

[nospame]

I think this looks good! Did leave some context/structural/reasoning questions.

[nospame]

It seems like get_app_builds is only used by get_latest_build_version_at_time. Rather than just retrying the query here, could that if not logic be moved (or removed) if get_app_builds is called directly by get_latest_build_version_at_time?

[jingcheng16]

Can you elaborate on this? Here what I'm trying to do is first get the latest 10 builds, and see if we can identify the latest version at use time from this 10 builds, if not, then query again to get all builds, and try to identify the latest version from all builds

[nospame]

It makes sense what this is trying to do, I think what I'm getting at is that I think the retry logic should be handled in the method doing the fetching, and doesn't need to be seen by _get_user_builds. Maybe I'm overcomplicating it/trying to overabstract things. But I'm imagining something like:

def get_latest_build_version_at_time(domain, app_id, time):
    builds = self.get_app_builds(domain, app_id, limit=self.INITIAL_QUERY_LIMIT)
    latest_build = self._get_latest_build_version_if_present(builds, time)

    if latest_build is None:
        builds = self.get_app_builds(domain, app_id, start=self.INITIAL_QUERY_LIMIT)
        latest_build = self._get_latest_build_version_if_present(builds, time)

    return latest_build

Where _get_latest_build_version_if_present (or whatever naming) is handling the loop and returning None if the latest isn't in the builds that were passed in, and get_app_builds is modified to take a start parameter or something similar, so it doesn't return the same initial 10 builds along with the rest of them. Then get_latest_build_version_at_time will always return the actual latest build version.

[gherceg]

+1 to separating out that logic. A helper method should be capable of doing everything to get the latest build version.

[jingcheng16]

Refactored in 25e5e52, also updated the docstring!

[nospame]

I don't love that this yields three unnamed items (as opposed to an object or namedtuple or something), but given it's a private method it's probably okay.

[gherceg]

Based on how it is used, you don't need to return the user right? And it might make a bit more sense to return build['build_version'] instead of build right? That would help make this more readable to Evan's point too.

                yield current_version, latest_version

[jingcheng16]

_get_user_builds is also used in

commcare-hq/corehq/apps/enterprise/enterprise.py

Lines 686 to 698 in 25e5e52

	for user, build, latest_version in self._get_user_builds(domain, apps):
	if is_out_of_date(str(build['build_version']), str(latest_version)):
	app_id = build['app_id']
	if app_id not in app_name_by_id:
	app_name_by_id[app_id] = Application.get_db().get(app_id).get('name')
	rows.append([
	user['username'],
	domain,
	app_name_by_id[app_id],
	latest_version,
	build['build_version'],
	self.format_date(DateTimeProperty.deserialize(build['build_version_date'])),
	])

. I need those info for each row. Maybe I can return username instead of user?

[mjriley]

At least for me, I think its that the return isn't particularly intuitive. I don't expect that I'll receive user, build, and latest_version from _get_user_builds. Given that it isn't obvious what the return value is, there might be some benefit to instead return a dictionary with those values clearly named -- so now _get_user_builds returns some collection of data, and the calling function explicitly referencing the named values makes what its doing a bit easier to follow?

[jingcheng16]

Alright! Returned a dictionary, also just return username instead of the whole user object in 4d009d5

[gherceg]

How did you settle on the number 10?

[jingcheng16]

I think normally the version should be not too out-of-date, so I thought 10 might be a good number...

[gherceg]

Based on how it is used, you don't need to return the user right? And it might make a bit more sense to return build['build_version'] instead of build right? That would help make this more readable to Evan's point too.

                yield current_version, latest_version

[gherceg]

+1 to separating out that logic. A helper method should be capable of doing everything to get the latest build version.

[mjriley]

Looks good. Mostly some minor suggestions that we could do to clarify some of the code, but do have some questions over whether some optimizations are necesssary

[mjriley]

Is this optimization necessary? Having this module know about the structure of an application document seems like a violation of the Law of Demeter. Using Application.get(app_id).name does not make as many assumptions. I get that using get_db() avoids the extra wrap call, but since we're caching in order to only ever fetch an application once, this seems like pre-mature optimization?

[jingcheng16]

Caching improved performance by 92%, avoid wrap application object further improved performance by 20%. So I think it is necessary.

[mjriley]

This definitely seems like the performance vs readability discussion. Now that it appears we no longer have the strict performance concerns (due to not populating the summary data), do you think we should emphasize the readability/maintainability side rather than the 20% performance gain?

[jingcheng16]

Can you explain more on why Application.get_db() have readability issue?

[mjriley]

This is the Law of Demeter, that I linked above. When modules start knowing about more than 1 level of internal structure of another module, things tend to get brittle -- unexpected errors crop up and the code is more difficult to maintain. I'm sure the link I provided above has a more full description. Application.get_db().get(app_id).get('name') means that this enterprise module essentially knows about the data access layer. Not just how to query the database (Application.get_db().get(app_id)) but also about how the database internally represents an Application. The application layer only generally wants to deal with fully constructed Application objects, which gives us flexibility -- name could be stored in an arbitrary manner in the database, but anyone interacting with the Application object wouldn't need to know those details. With this code, we do.

[mjriley]

At least for me, I think its that the return isn't particularly intuitive. I don't expect that I'll receive user, build, and latest_version from _get_user_builds. Given that it isn't obvious what the return value is, there might be some benefit to instead return a dictionary with those values clearly named -- so now _get_user_builds returns some collection of data, and the calling function explicitly referencing the named values makes what its doing a bit easier to follow?

[mjriley]

I think these kind of docstrings don't add much value. The function is named get_latest_build_version_at_time, which means the description of 'Get the latest build version available at the given time' is redundant. domain is supposed to be standardized across our codebase to always represent the domain name as a string. app_id is always a string. get_latest_build_version_at_time suggests what time represents.
The return value is somewhat useful, as it seems we sometimes represent builds as strings, and othertimes as tuples -- but this docstring doesn't clarify that (and is something we should probably standardize without a comment)

Basically, we haven't standardized docstrings across our codebase, and I think we want to be a bit selective about only using them when needed

[biyeun]

from review: rename to get_latest_build_version, and rename last parameter to at_datetime and remove docstring? unless folks have other ideas.

[jingcheng16]

5bb7b5a

[mjriley]

Nit: I am curious if this would be better written in the reverse -- i.e. immediately return the cached build if it exists, otherwise go to elasticsearch. The downside to this approach is that it indents all the complicated logic. I think we also tend to think of cached data as performing this way -- "Is the value cached? Return it. Otherwise, look up the data and return it"

[jingcheng16]

c79f198

[mjriley]

Would this logic be better in its own function? This function has to do the loop comparison as well as handling the details of generating and caching build info. Seems like it might be cleaner to move the latter portion into a get_build_info function.

[biyeun]

create new function get_build_info from build doc for lines 776 to 788

[jingcheng16]

8a4accd

[mjriley]

There is quite a bit of logic here that doesn't really interact with our databases. I'd like to see tests for this kind of thing in the future. I know personally I've overlooked edge cases when I don't write tests covering the different types of inputs. The other benefit is that these tests serve as documentation, which often make the docstrings here unnecessary -- people can look at tests to understand how the function can be used.

[biyeun]

from review: rename time to at_datetime, remove doctstring. add tests

[jingcheng16]

rename and removal in 5bb7b5a
I think the logic is relatively straightforward. It basically checking a cache, determining a release date, and comparing dates, then return the version. I did a refactor in 936c43e. Hope it's better.

[mjriley]

total_last_builds was confusing to me. Would total_builds convey the same thing? It makes more sense to me to think of how many total builds we have, and how many total builds are out of date.

[jingcheng16]

Thanks! 3a271f1

[biyeun]

looks like this was updated in a different location

[jingcheng16]

This is removed in 12a4ff6 since we're not displaying metric

[mjriley]

Do we need this optimization? My guess is that applications don't get more than a few hundred releases in the worst case, and even in that worst case, we're only fetching _id, version, last_released, and build_on. Essentially, I would think the time difference between fetching this data for all builds vs just the 10 most recent is negligible, and would allow us to simplify the code (no limit or start).

That said, if you did performance testing and needed to do this optimization, that's probably a great thing to document.

[jingcheng16]

I did this experiment:

Without the initial query limit, always fetching all data at the first time: 1208 es queries, 45 sec
I plan to change Initial Query Limit from 10 to 15 though based on the experiment.
Where should I document it?

[jingcheng16]

But since we are not displaying a metric in front end. I guess I can remove that logic too. 08443df

[mjriley]

I think I have some remaining questions on our performance tradeoffs and some of the caching

[mjriley]

Can you explain the role of build_info_cache here? It looks like we're already caching the raw results from the elasticsearch query. Then, within _find_latest_build_version_from_builds, we just want to turn that raw doc into something we can use to determine whether or not this build's release date was prior to the datetime specified. A conversion function doesn't sound like it would need a cache. Was this something that came up while profiling? If we are caching these results to avoid repeated calls to DateTimeProperty.deserialize, perhaps that is something we can do when we first cache the elasticsearch results?

[jingcheng16]

build_info_cache is to cache all required information of a build, mainly version number and last released date. I did this because I realized deserialize also have an impact on performance, so I want to do less deserialization.

perhaps that is something we can do when we first cache the elasticsearch results?

This will result in deserialize unnecessary build, for example, if an app already have 1000 builds, then it is very likely the first 500 build's last released date will never be required in the comparison then no need to deserialize.

[mjriley]

If you haven't seen it yet, there's an infamous statement about "Premature optimization is the root of all evil".

I wrote up a quick performance test to gauge how big of an impact deserialization had:

from datetime import datetime
from dimagi.utils.dates import safe_strftime
from dimagi.utils.parsing import ISO_DATETIME_FORMAT
from dimagi.ext.couchdbkit import DateTimeProperty
import random

TIMESTAMP_FLOOR = 1_000_000_000
TIMESTAMP_CEIL = 1_730_000_000

def create_datetime_string():
    return safe_strftime(
        datetime.fromtimestamp(
            random.randrange(TIMESTAMP_FLOOR, TIMESTAMP_CEIL)
        ), ISO_DATETIME_FORMAT)

def create_n_dates(n):
    return [create_datetime_string() for i in range(n)]

def time_deserialize():
    num_elements = 1000
    all_dates = create_n_dates(num_elements)
    deserialized_dates = [None] * num_elements

    start = datetime.now()
    for i in range(num_elements):
        deserialized_dates[i] = DateTimeProperty.deserialize(all_dates[i])
    end = datetime.now()

    print(f'Took: {end - start}')

TL;DR -- the time to deserialize 1K datestrings, on my machine, took 10K to 15K microseconds -- i.e. milliseconds.

From the current code, it appears build_info_cache only cares about the build number and last released date -- I don't see any other data being used. Given that deserialization has virtually no performance impact, I think it makes the code cleaner for get_app_builds to return only the needed information (version, last_released_date) and do the deserialization immediately. This allows us to get rid of build_info_cache

[jingcheng16]

I finally find the commit where I decide to deserialize separately without wrapping the raw document: f94b4b3
I think because I cached the wrapped result initially, so when I change to only deserialize the datetime without wrapping it, I never thought of getting rid of the cache.
I will put up a PR to remove the cache.

[mjriley]

This definitely seems like the performance vs readability discussion. Now that it appears we no longer have the strict performance concerns (due to not populating the summary data), do you think we should emphasize the readability/maintainability side rather than the 20% performance gain?

[mjriley]

I don't know if this is something you need to act on, but wanted to bring it up -- I think I struggled to figure out what was going on here, because _get_user_builds doesn't sound like it's going to be returning latest version information, and because row_data doesn't really suggest what is being dealt with.

In pseudo-code, I'd imagine this is what we're trying to do:

retrieve app-last-usage information for all users in a domain
for each app-last-usage:
  retrieve the most current version at the time the app was last used
  if that version is not the same version as is in our information, then
    add a row to our report with the relevant details

I'm not sure the above is any better, but if you feel there's some confusion with what is going on with row_data, perhaps its worth addressing

[jingcheng16]

I think I rename it to row_data because build['build']['build_version] looks weird. What do you think of change line 675 to for build_and_latest_version in self.all_last_builds_with_latest_version(domain, app_ids):

[mjriley]

That seems like a good improvement

[jingcheng16]

Thanks! df144d4

[mjriley]

I'm approving to allow this tile to be live on production. I'm convinced this code works, but I hope we can clean up this code in the future to make it more readable and maintainable.