openedx / edx-django-utils

edX utilities for Django Application development.
https://edx.readthedocs.io/projects/edx-django-utils/en/latest/
Apache License 2.0
26 stars 20 forks source link

WIP: ARCH-177: cache and monitoring #1

Closed robrap closed 6 years ago

robrap commented 6 years ago

Putting up for an early review, even though it is very broken, because I want to work out structure before making lots of fixes.

robrap commented 6 years ago

@nasthagiri @ormsbee: I'd love some early directory structure/naming feedback before I fix code so it runs.

See the following directory and then review the following questions.

Thanks!

ormsbee commented 6 years ago

Are we ok with the conf directory being mixed in with the other utility directories?

Do we need it at all? Do we even need this to be an installed app with an apps.py as opposed to a library? We don't declare any models, right?

I don't love it, but I think from edx_django_utils.monitoring import set_custom_metric may be better than from edx_django_utils.XXX.monitoring import set_custom_metric for any 'XXX' I could think of. Thoughts?

I agree.

These apps were named cache_utils and monitoring_utils. I dropped _utils because it is in edx_django_utils. I'm not sure how I feel about this. Thoughts?

+1

Should cache be renamed to caching for consistency, or fine as-is?

I think it's fine as is, but perhaps we should change "monitoring" to "metrics"? Anyhow, I have no strong opinion either way.

Original cache_utils had a utils.py and an empty init.py, where monitoring_utils used init.py. I personally don't usually love code in init.py, but I guess it is a question of whether we would rather have the import written above, or have from edx_django_utils.monitoring.utils import set_custom_metric? I think it would be nice if cache and monitoring, and things to follow, generally followed the same pattern.

Agree that consistency is good. A common thing you could do is to still write all the code in a submodule and then have an __init__.py that just imports whatever the public API classes/functions are.

robrap commented 6 years ago

Thanks @ormsbee.

  1. I didn't really review all the cookie-cutter code, and didn't realize that conf was just django app conf. I'll kill it.
  2. I'll rename monitoring to metrics.
  3. I like the last idea regarding the Public API. I can give that a whirl.