Replay "ARCHBOM-1494: Refer to custom attributes, not metrics, especially with edx-django-utils" - #25055
Conversation
|
Your PR has finished running tests. There were no failures. |
|
Pending a conflict in another release. |
| @patch('edx_django_utils.monitoring.set_custom_metric') | ||
| def test_access_token_metrics(self, token_type, expected_token_type, mock_set_custom_metric): | ||
| @patch('edx_django_utils.monitoring.set_custom_attribute') | ||
| def test_access_token_metrics(self, token_type, expected_token_type, mock_set_custom_attribute): |
There was a problem hiding this comment.
Good finds. Mind if I pick those up in a second PR so that this can be a clean replacement?
There was a problem hiding this comment.
Go for it. Do you want to start that PR now based off this branch?
There was a problem hiding this comment.
Oh, I was thinking of just merging this one and starting a new branch to keep things simple (since this one is going to get squashed).
There was a problem hiding this comment.
...but I'm not merging this until Monday anyhow, so I might as well start there and rebase later. :-)
| @patch('openedx.core.djangoapps.waffle_utils.set_custom_metric') | ||
| def test_waffle_flag_metric_for_various_settings(self, data, mock_set_custom_metric): | ||
| @patch('openedx.core.djangoapps.waffle_utils.set_custom_attribute') | ||
| def test_waffle_flag_metric_for_various_settings(self, data, mock_set_custom_attribute): |
There was a problem hiding this comment.
Handling in separate PR. See other thread: https://github.com/edx/edx-platform/pull/25055#discussion_r495174789
There was a problem hiding this comment.
This one is already covered in https://github.com/edx/edx-platform/pull/25018 as it turns out.
| edx-django-utils | ||
| # edx-django-utils 3.8.0 renames metric -> attribute and deprecates the old | ||
| # methods; edx-platform calls the new names | ||
| edx-django-utils>=3.8.0 |
There was a problem hiding this comment.
Nit: I prefer something shorter like https://github.com/edx/edx-drf-extensions/blob/master/requirements/base.in#L7, but either works.
There was a problem hiding this comment.
That seems nicely terse. May still keep it on a separate line since we conventionally use the same-line comment to describe the purpose/use of the dep.
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
|
EdX Release Notice: This PR has been rolled back from the production environment. |
4 participants
Reverts edx/edx-platform#25025 to replay https://github.com/edx/edx-platform/pull/25010 now that pipeline is unblocked.