-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add middleware DatadogDiagnosticMiddleware
#735
Conversation
Adds logging diagnostics for traces in Datadog. See #692
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering when and where it might help to have the most basic of unit tests? I know you probably did local testing, but sometimes a really simple unit test can just keep things happy when there are seemingly small changes made.
Otherwise, looks good.
# .. toggle_warning: This is a noisy feature and so it should only be enabled | ||
# for a percentage of requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we are dealing with both Splunk and Datadog, so this makes sense. Also, our Log Ingest was high (over-budget), but SRE may have taken care of that for us? Otherwise, in the future, we could instead control this via Log Index rules that could be tuned based on noisiness. Although the log messages might still affect disk usage, if it really is high.
Hmm... it would require a significant amount of test harness to test the span and resolver logic, and there aren't easy ways to test both the datadog-installed and datadog-missing branches. Honestly, I'm pretty happy to fall back on the try/except here to protect against exceptions and just rely on manual testing for the rest. In case you're curious about log size and what information is in the spans, here's a sample log line from devstack:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mocks are usually pretty simple to set up, although I know they are imperfect.
That said, it's not a blocker and at least I brought it up and had you respond. I agree that the exception handling is nice, but a test could ensure that your exception handler works as expected and doesn't have its own bug. ;)
Yeah, I could add |
Adds logging diagnostics for traces in Datadog.
See #692
Merge checklist:
Check off if complete or not applicable: