Skip to content

Conversation

@jwhitlock
Copy link
Member

@jwhitlock jwhitlock commented Apr 9, 2024

This refreshes the Django docs, and adds some information that may be useful.

  • Add that the DockerflowMiddleware also serves the __version__, __heartbeat__ and __lbheartbeat__ views. In mozilla/fx-private-relay PR #4345, we removed some duplicate versions of the views that were not called because of the middleware.
  • Document that check results are logged to dockerflow.checks.register. This is how you can see what checks are failing if DEBUG=False.
  • Add an example heartbeat response when DEBUG=False.
  • Add dockerflow to the sample logging configuration. Without this (and without a root handler), these logs are discarded, making it impossible to debug heartbeat issues.
  • Drop reference to MIDDLEWARE_CLASSES, which was removed in Django 2.0 in 2017.
  • Add SECURE_REDIRECT_EXEMPT for Kubernetes deployments.
  • Expand the Settings section to document all the added settings and a few Django ones as well:
    • Add Django's setting DEBUG, to document how it affects what checks heartbeat runs, and the data included in the heartbeat response.
    • Add DOCKERFLOW_HEARTBEAT_FAILED_STATUS_CODE, previously mentioned in the heartbeat view section
    • Add DOCKERFLOW_REQUEST_ID_HEADER_NAME, previously mentioned in the "Requests Correlation ID" section. Add why you may want to change it for different deployment environments.
    • Add DOCKERFLOW_SUMMARY_LOG_QUERYSTRING, previously mentioned in the Logging section.
    • Move DOCKERFLOW_VERSION_CALLBACK down to alphabetical order
    • Add Django's setting SILENCED_SYSTEM_CHECKS, used but not previously documented.
    • I did not add BASE_DIR, since there is no Django docs, because Django maintainers say that is a constant, not a setting (https://code.djangoproject.com/ticket/31387).

@jwhitlock jwhitlock requested a review from leplatrem April 9, 2024 20:42
These logs should propagate up, so use more general logging level.
@jwhitlock
Copy link
Member Author

I decided to use dockerflow instead of dockerflow.checks.register in the sample logging config. This should have the same effect, but allow moving files around or catching logs from other locations. I also fixed the name of RequestIdLogFilter in the example.

@leplatrem
Copy link
Contributor

Thanks @jwhitlock for the contribution 👏

We will have to postpone the review for a few days 🙏

leplatrem
leplatrem previously approved these changes Apr 16, 2024
Copy link
Contributor

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! TIL about traceparent, we could consider making it the default :)

docs/django.rst Outdated
The check setup process is logged at the ``DEBUG`` level. Since failure
details are omitted with ``DEBUG=False``, this logger should emit logs
at ``WARNING`` or ``ERROR`` level in production, so that the logs can
be used to diagnose heartbear failures.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
be used to diagnose heartbear failures.
be used to diagnose heartbeat failures.

docs/django.rst Outdated

A unique request ID is read from the ``X-Request-ID`` request header, and a UUID4 value is generated if unset.

Leveraging the ``RequestIdFilter`` in logging configuration as shown above will add a ``rid`` field into the ``Fields`` entry of all log messages.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine that this should be RequestIdLogFilter here too

Unfortunately git grep RequestIdFilter other results :( We'll fix them in a follow-up (unless you want to do them as driveby)

@jwhitlock
Copy link
Member Author

Thanks @leplatrem, I've applied the changes, including RequestIdLogFilter across the project. I've been focused on the Django docs, I see others are better about updating all the docs.

traceparent does look interesting to me. I'd want to see how GCP implements it before making it the default, and maybe talk to the SREs about how we'd integrate it.

@jwhitlock jwhitlock requested a review from leplatrem April 16, 2024 14:24
@leplatrem leplatrem merged commit 0424734 into mozilla-services:main Apr 16, 2024
@leplatrem
Copy link
Contributor

Thank you!

@jwhitlock jwhitlock deleted the update-django-docs-jwhitlock branch April 18, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants