-
Notifications
You must be signed in to change notification settings - Fork 581
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 datadog tracing to http middleware #530
Conversation
Codecov Report
@@ Coverage Diff @@
## main #530 +/- ##
==========================================
+ Coverage 63.56% 63.86% +0.30%
==========================================
Files 197 114 -83
Lines 11524 10654 -870
Branches 85 0 -85
==========================================
- Hits 7325 6804 -521
+ Misses 3419 3091 -328
+ Partials 780 759 -21
Continue to review full report at Codecov.
|
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.
Two minor things, but LGTM!
The failing check on here will be fixed if you merge in |
1083544
to
a16dc3e
Compare
Moved the ddtracer into a thin wrapper package |
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.
Let's add a thin-test to call start and stop with goleak. Then we can ensure no goroutines are leaked!
Apart from my one structural comment, this is awesome! Thoughts on adding a CLI flag to enable/disable tracing with the DATADOG_TOKEN
? I'd rather this not be implicit, just in case customers want to disable it.
We can add that to coder.env
too. That's used by our systems service in our dogfood deployment! Tracing in dogfood😎
This reverts commit 9ce5328.
@f0ssel it appears the tunnel default to true broke from the flag PR. Since this is about to merge, could you investigate that? (assuming it's a simple fix) |
I'll do this in a separate PR to keep things clean |
No description provided.