Report app dependency addition/removal to DataDog #35455

Charl1996 · 2024-11-28T13:52:50Z

Relates to this PR

Technical Summary

This PR increments a counter on DataDog whenever the app dependencies feature has been disabled for a build which had it enabled which allows us to track the feature drop-out rate. We don't need to know any domain information, just the overall counter.

Note that the counter is incremented during the make_build phase.

(The related PR will be changed to support a management command which will be run to "top up" the counter with historic data)

Safety Assurance

Safety story

Tested locally as far as I could. Tested on staging as well.

Automated test coverage

No automated tests included.

QA Plan

No formal QA will be done

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

zandre-eng · 2024-11-29T07:36:37Z

corehq/apps/app_manager/models.py


        latest_build = get_latest_build_doc(self.domain, self.id)
+        if not latest_build:


When would there not be a latest build?

The "latest_build" here refers to the previous build, as the new build has not been made at this point. So the "latest_build" could be None if it's the first build.

Ah I see, thanks for the clarification

nit: to make it slightly better, you could consider naming this last_build.

ajeety4

LGTM

mkangia · 2024-12-03T09:22:47Z

corehq/apps/app_manager/models.py

+            return
+
+        if has_dependencies(latest_build) and not has_dependencies(new_build):
+            metrics_counter('commcare.app_build.dependencies_removed')


I like that we have simplified the approach here.

Wanted to check if this metric in itself would be useful? Don't we need the uptake as well?
I believe on datadog you can have a counter with both positive and negative values, so you get a graph with both additions and removals at once.
example here

Or we could report uptake separately, which seems easy enough to add now.

if (not latest_build or not has_dependencies(latest_build)) and has_dependencies(new_build): metrics_counter('commcare.app_build.dependencies_added')

I would vote for the single counter with both +/-

I believe on datadog you can have a counter with both positive and negative values, so you get a graph with both additions and removals at once.

I'll check this out. Thanks for the suggestion.

So, these will show as two separate metrics right? not as one? or they can be combined if needed on the datadog UI?

@mkangia They can be combined as needed on Data Dog.

mkangia · 2024-12-04T12:55:46Z

corehq/apps/app_manager/models.py

        return copy

+    def check_build_dependencies(self, new_build):
+        """
+        Reports whether the app dependencies have been removed.


looks like the documentation needs to be updated.
And possibly the function name as well.

mkangia · 2024-12-04T12:57:03Z

corehq/apps/app_manager/models.py

+        """
+
+        def has_dependencies(build):
+            if isinstance(build, Application):


wont it always be Application object since we wrapped it?
Its unclear why we are checking this here.

The new_build is wrapped and will be an instance of Application, but the last_build is not (see here). I suppose I have two options:

Also make sure to wrap the last_build results and remove the instance checking

Leave as is and handle different instance types.

Any preference from your side?

Actually, no, I'm going to wrap it as well, because I think the fact that you had to ask this question makes the code less obvious as it is now.

mkangia

Looking good 👍

mkangia · 2024-12-05T15:02:24Z

corehq/apps/app_manager/models.py

            )

        new_build_has_dependencies = has_dependencies(new_build)

        last_build = get_latest_build_doc(self.domain, self.id)
+        last_build = ApplicationBase.wrap(last_build) if last_build else None


nit: Not sure the difference but should this be cls.wrap? just like when the copy is created.

This is in instance method, so either self.__class__.wrap or ApplicationBase - I prefer the latter.

Will that impact if the function is called by inherited classes instead?

Yes, if a subclass calls this method that would mean the ApplicationBase wrap method is still called and not the subclass's wrap method. I think this is OK, since we don't use anything except the profile attribute. But that is a good point, maybe it should take the subclass's own implementation of wrap?

ApplicationBase wrap method is still called and not the subclass's wrap method

Ya. We would be wrapping data for a different class on ApplicationBase which isn't ideal and would further lead to issues if the subclass adds more properties than ApplicationBase, which will get skipped leading to loss of data in case a save is performed.

Good point. I'll update

You might check out wrap_app for this.

@orangejenny What is the exact benefit of using wrap_app(last_build, self.__class__) instead of simply self.__class__.wrap(last_build)? I see the wrap_doc function have the get_correct_app_class addition, but that's only coming into play if I don't pass in the class name, which I will do in this instance.

Probably doesn't matter, then. I had just read the original code when making the suggestion.

wrap_app looks like a nice catch all for different application and similar docs with a hard failure in case of unexpected scenarios. Definitely good to use when dealing with more complex code.
The class wrap seems safe here since we are passing a build explicitly.

mkangia

Looks good to me.

Can we add some tests?

Charl1996 · 2024-12-10T06:26:54Z

Can we add some tests?

I can add some. Didn't think it was necessary since it's an easy thing to test locally, but for peace of mind I'll add some.

…commcare-hq into cs/SC-4053-add-metric-to-datadog

mkangia

Thank you @Charl1996

Are we to release this after populating the datadog dashboard? so you will merge this only after it?

Charl1996 · 2024-12-10T08:55:20Z

@mkangia No it's not dependant on this PR; the "top up" only increments the counter to bring it up to speed (so to speak).

mkangia · 2024-12-10T16:53:03Z

💯

Report app dependency removal

2056cc2

Charl1996 requested review from kaapstorm, orangejenny, mkangia, zandre-eng and ajeety4 November 28, 2024 13:52

Patch up method

7179077

zandre-eng approved these changes Nov 29, 2024

View reviewed changes

Consider that build can be Application instance

a94b241

zandre-eng approved these changes Nov 29, 2024

View reviewed changes

ajeety4 approved these changes Nov 29, 2024

View reviewed changes

mkangia reviewed Dec 3, 2024

View reviewed changes

Charl1996 mentioned this pull request Dec 4, 2024

Add management command for updating Data Dog metric #35473

Closed

3 tasks

Charl1996 added 2 commits December 4, 2024 09:37

Rename var

c2ef0d0

Send metric for app dependecies feature added to build

af3a6a1

mkangia reviewed Dec 4, 2024

View reviewed changes

Charl1996 added 2 commits December 5, 2024 07:55

Update docstring

08b1425

Wrap unwrapped last_build app doc

1823cbd

kaapstorm approved these changes Dec 5, 2024

View reviewed changes

mkangia reviewed Dec 5, 2024

View reviewed changes

Charl1996 and others added 2 commits December 9, 2024 10:51

Use calling class's own wrap method

319593f

Merge branch 'master' into cs/SC-4053-add-metric-to-datadog

293fcaa

mkangia reviewed Dec 9, 2024

View reviewed changes

Charl1996 added 3 commits December 10, 2024 08:36

Add some tests

1c71a7b

Merge branch 'cs/SC-4053-add-metric-to-datadog' of github.com:dimagi/…

cbd65ab

…commcare-hq into cs/SC-4053-add-metric-to-datadog

Merge branch 'master' into cs/SC-4053-add-metric-to-datadog

aee1a9a

mkangia approved these changes Dec 10, 2024

View reviewed changes

Charl1996 merged commit 155b20c into master Dec 10, 2024
13 checks passed

Charl1996 deleted the cs/SC-4053-add-metric-to-datadog branch December 10, 2024 12:11

mkangia changed the title ~~Report app dependency removal to DataDog~~ Report app dependency addition/removal to DataDog Dec 10, 2024


		latest_build = get_latest_build_doc(self.domain, self.id)
		if not latest_build:

Report app dependency addition/removal to DataDog #35455

Report app dependency addition/removal to DataDog #35455

Conversation

Charl1996 commented Nov 28, 2024 • edited Loading

Technical Summary

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

Labels & Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeety4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkangia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkangia left a comment

Choose a reason for hiding this comment

Charl1996 commented Dec 10, 2024

mkangia left a comment

Choose a reason for hiding this comment

Charl1996 commented Dec 10, 2024

mkangia commented Dec 10, 2024

Charl1996 commented Nov 28, 2024 •

edited

Loading

Charl1996 Dec 9, 2024 •

edited

Loading