-
Notifications
You must be signed in to change notification settings - Fork 532
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
Set level based on status code for HTTP client breadcrumbs #4004
Set level based on status code for HTTP client breadcrumbs #4004
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #4004 +/- ##
==========================================
+ Coverage 80.16% 80.22% +0.05%
==========================================
Files 139 139
Lines 15397 15406 +9
Branches 2597 2601 +4
==========================================
+ Hits 12343 12359 +16
+ Misses 2210 2202 -8
- Partials 844 845 +1
|
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.
Just a tiny nit picking question, otherwise looks good!
if level: | ||
scope.add_breadcrumb( | ||
type="http", category="httplib", data=span._data, level=level | ||
) | ||
else: | ||
scope.add_breadcrumb(type="http", category="httplib", data=span._data) |
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.
Does it mess up the breadcrumb if we would add it with level=None
?
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 don't think but wasn't entirely sure so opted for the safe version 😬
maybe_create_breadcrumbs_from_span
to set thelevel
of the breadcrumb towarning
for the client error range (4xx) and toerror
for server errors (5xx)responses
in multiple places, but they're not actually active (theactivate
decorator is missing) and we're making actual requests outside -- we should clean this upresponses
for stdlib/requests tests since they patch something that we patchhttpx
,stdlib
,requests
,aiohttp
tests for the new behaviorrequests
tests to 3.7+ since in 3.6, the span is finished before the HTTP status is set for some reason...Closes #4000