Check for custom callouts on app builds #35628
Conversation
Yeah, I think this should go in a celery task. My recollection is that
I agree the new metric isn't likely to incur significant cost, but just for the exercise, you could use analytics to confirm the actual frequency of this action. We should have a metric in GA for how often this button is clicked. |
corehq/apps/app_manager/models.py
Outdated
|
|
||
| return copy | ||
|
|
||
| def analyse_app_build(self, new_build): |
There was a problem hiding this comment.
+1 on doing this async.
Gathering metrics for our use should not delay user interaction, seems unreasonable.
corehq/apps/app_manager/models.py
Outdated
| if app_has_custom_intents(): | ||
| metrics_counter( | ||
| 'commcare.app_build.custom_app_callout', | ||
| tags={'domain': new_build.domain, 'app_id': new_build.copy_of}, |
There was a problem hiding this comment.
Just checking on this, same as the other PR, does adding tags effect cost on datadog and how? I assume a new custom metric is a new cost of its own.
There was a problem hiding this comment.
I see there is context about this in the description.
So, we are saying since making new version happens infrequently and that we are reporting only when its applicable this should be okay, seems fair.
I see Jenny already noticed this PR, but Just tagging @gherceg as well for visibility that a new custom tag is being added.
(let me know if its not necessary for us to notify you or someone everytime a new tag is added)
There was a problem hiding this comment.
I still want to check out how often a new build is created so we can at least have a sense of frequency. Will update when I know.
corehq/apps/app_manager/models.py
Outdated
| def check_for_custom_callouts(self, new_build): | ||
| from corehq.apps.app_manager.util import app_callout_templates | ||
|
|
||
| templates = next(app_callout_templates) |
There was a problem hiding this comment.
should this be list instead of next?
There was a problem hiding this comment.
The app_callout_templates variable holds a reference to a function which yields, so maybe next is a better suit here?
|
I moved the |
WalkthroughThe pull request introduces changes to the app build process across multiple files in the The changes implement a new asynchronous task The changes affect how app builds are created and analyzed, introducing a more modular approach to tracking and reporting on app build characteristics. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
corehq/apps/app_manager/tests/test_apps.py (1)
42-42: Consider refactoring to avoid importing from views in testsImporting
make_app_buildfromcorehq.apps.app_manager.views.releasesintotests.test_apps.pymay mix concerns between views and tests. To maintain a clean separation of concerns, consider refactoringmake_app_buildinto a utility module or the model layer, and import it from there.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
corehq/apps/app_manager/models.py(0 hunks)corehq/apps/app_manager/tasks.py(3 hunks)corehq/apps/app_manager/tests/test_apps.py(4 hunks)corehq/apps/app_manager/views/releases.py(2 hunks)
💤 Files with no reviewable changes (1)
- corehq/apps/app_manager/models.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tests (python-sharded-and-javascript)
- GitHub Check: tests (python, bf)
- GitHub Check: tests (python, 6a)
- GitHub Check: Lint Javascript
- GitHub Check: tests (python, 05)
- GitHub Check: test
| previous_build_id = app_build_ids[app_build_ids.index(new_build.id) + 1] | ||
| previous_build = get_app(new_build.domain, previous_build_id) |
There was a problem hiding this comment.
Potential IndexError when accessing previous build ID
In check_build_dependencies, lines 142-143 calculate previous_build_id using app_build_ids[app_build_ids.index(new_build.id) + 1]. If new_build.id is the last element in app_build_ids, this will result in an IndexError. Additionally, if new_build.id is not in app_build_ids, a ValueError will be raised.
Modify the code to handle these edge cases safely:
if len(app_build_ids) > 1:
try:
idx = app_build_ids.index(new_build.id)
if idx + 1 < len(app_build_ids):
previous_build_id = app_build_ids[idx + 1]
previous_build = get_app(new_build.domain, previous_build_id)
last_build_has_dependencies = has_dependencies(previous_build) if previous_build else False
except (ValueError, IndexError):
# Handle the case where new_build.id is not found or is the last element
last_build_has_dependencies = FalseCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@Charl1996 Would you like me to help you implement the suggested fix to handle the edge cases safely? Let me know if you need any assistance.
There was a problem hiding this comment.
No, it's fine. I don't think this will be a concern because the docstring in the get_build_ids says the following:
"Returns all the built apps for an application id, in descending order of time built."
I've verified that it's indeed so, meaning
- the
new_build.idwill definitely exist in the list - if it's the last item in the list it also means its the only item in the list and the
len(app_build_ids)will evaluate to false, meaning this block of code won't execute in the first place.
There was a problem hiding this comment.
@Charl1996 I've found the implementation in dbaccessors.py and can confirm your explanation. The docstring indeed explicitly states that it "Returns all the built apps for an application id, in descending order of time built." This validates your assessment of the code's safety. Thank you for helping me understand the implementation better.
🔗 Analysis chain
@Charl1996 Thank you for the detailed explanation. I understand now that the code is indeed safe due to the guarantees provided by get_build_ids. Your explanation makes perfect sense - since the builds are returned in descending order and the new build will always be present, we don't need additional error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of get_build_ids
ast-grep --pattern 'def get_build_ids'
# Also search for any documentation or comments about get_build_ids
rg -A 5 'def get_build_ids'
Length of output: 1548
✏️ Learnings added
Learnt from: Charl1996
PR: dimagi/commcare-hq#35628
File: corehq/apps/app_manager/tasks.py:142-143
Timestamp: 2025-01-20T08:22:44.938Z
Learning: The `get_build_ids` function in corehq.apps.app_manager.dbaccessors returns all built apps for an application ID in descending order of build time, which guarantees that:
1. A newly built app's ID will always exist in the returned list
2. If the new build ID is the last item in the list, it means it's the only item, preventing any IndexError in the `check_build_dependencies` function
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Here's a carrot, Mr Rabbit!
There was a problem hiding this comment.
@Charl1996 🥕 Thank you for the carrot! happy rabbit noises 🐰
There was a problem hiding this comment.
I agree this shouldn't be an issue, but you could save readers from going through the same thought process you outlined, and make it obvious that isn't an issue, by doing something like:
build_index = app_build_ids.index(new_build.id)
if build_index < len(app_build_ids):
previous_build_id = app_build_ids[build_index + 1]
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@CodeRabbit Please summarize the things you have learned from contributers so far about the codebase and the way we do things. |
|
Tip For best results, initiate chat on the files or code changes. @Charl1996 Based on the discussions in this PR, here are the key learnings about the codebase and development practices:
✏️ Learnings added
|
Can you say more about this? It looks like there's still the possibility that if |
Ah, that's a good point!
Sounds like a good idea. Let me update. |
corehq/apps/app_manager/tasks.py
Outdated
|
|
||
|
|
||
| def check_for_custom_callouts(new_build): | ||
| from corehq.apps.app_manager.util import app_callout_templates |
There was a problem hiding this comment.
It might be worth caching app_callout_templates if it's now going to be called often.
…agi/commcare-hq into cs/SC-4113-check-for-custom-callouts
corehq/apps/app_manager/util.py
Outdated
| @@ -540,7 +541,13 @@ def _app_callout_templates(): | |||
| yield data | |||
|
|
|||
|
|
|||
| @memoized | |||
There was a problem hiding this comment.
Memoized is only an in-memory cache, so it isn't the thing to use here, you'll want to cache across requests. I was thinking you could quickcache, probably for a while, maybe a week or longer. Check out these docs if you haven't seen them.
There was a problem hiding this comment.
Memoized is only an in-memory cache ... you'll want to cache across requests
Ah, I thought it was the other way around (but haven't read that docs to be honest). Probably the "quick" in quickcache that got my mind on the wrong track :shrug
There was a problem hiding this comment.
cool, thanks for updating
|
The test error is as follows:
Observation: Redis lives at I've followed these steps but the output (and error log) is clean - no errors. |
Technical Summary
Ticket
This PR adds some more insights into how users use the app dependencies feature in an application (or rather, how they don't use it).
The broader intent is to find out what level of adoption we have for the app dependencies feature and if any projects make use of app callouts but don't use the app dependencies feature. This will enable us to start asking "why not".
Notes
Celery task
I think the new additional method should be a relatively cheap one to process, but I have had some thoughts of handing the
analyse_app_buildmethod off to celery for async processing, since reporting analytics shouldn't be a blocking operation. Any input/thoughts here would be welcome.Datadog custom metric
Prereading
Reporting the app ID and domain will count as additional custom metrics on Datadog (for which
app_idwould be the biggest contributer to increased metric variance), but...I think the variance is OK given the low frequency of metric reporting.
Safety Assurance
Safety story
Tested locally. Staging testing to commence.
Automated test coverage
No automated testing
QA Plan
Going through QA
Rollback instructions
Labels & Review