Document Expected Features for the HTTP client integrations #341
Conversation
|
@maciejwalkowiak is attempting to deploy a commit to the Sentry Team on Vercel. A member of the Team first needs to authorize it. |
|
cc @PeloWriter for some reason can't add you as a reviewer |
|
I'm not a reviewer on the developer site, but happy to add comments if you ping me (as you did for this). |
Co-authored-by: Fiona <[email protected]>
Co-authored-by: Fiona <[email protected]>
src/docs/sdk/features.mdx
Outdated
|
|
||
| - HTTP request must be enhanced with a [`sentry-trace` HTTP header](/sdk/performance/#header-sentry-trace) | ||
| - span status must match HTTP response status code ([see Span status to HTTP status code mapping](/sdk/event-payloads/span/)) | ||
| - when network error occurs, span status must be set to `INTERNAL_SERVER_ERROR` |
There was a problem hiding this comment.
What is meant by "network error" here? There are many things that can happen like failed to resolve DNS, timeout, etc.
Note that INTERNAL_SERVER_ERROR is not a valid status (internal_error is).
Lacking more context, I guess unknown_error is more appropriate as a generic catch-all status, but I look forward to us simplifying the statuses in the near future, the current list is confusing and not well fitting the product.
There was a problem hiding this comment.
@rhcarvalho about the unknown_error , See -> #307 (comment)
we've changed it because the description of unknown_error is actually:
An unknown error raised by APIs that don't return enough error information
and in our case, network error is eg you've no connection, something happened that you could not really talk with the endpoint, that's why internal rather unknown.
There was a problem hiding this comment.
I agree that status are a bit confusing, but that's what we've right now, we can revisit this in the future
There was a problem hiding this comment.
I see. Reusing internal_error unfortunately confuses with a 500 response.
There was a problem hiding this comment.
indeed, but unknown_error is also 500 anyway, so it's not any better IMO, at least internal_error fits better when you read the desc. under https://develop.sentry.dev/sdk/event-payloads/span/ status section
There was a problem hiding this comment.
To be sure we understand the same, while the status values (first column, States) is sort of fixed (known by Relay), the Description and HTTP status code equivalent are under our control; it is whatever we want to document and it is relatively easy to change. AFAIK nothing in the product depends on those descriptions.
There was a problem hiding this comment.
yep, but right now that's how its implemented in some of the SDKs (using inetrnal_error) and its the one that fits the most in the description https://develop.sentry.dev/sdk/event-payloads/transaction/
so unless we fix the SDKs and the status descriptions, let's keep it like this, I'll merge as it is.
bruno-garcia
left a comment
bruno-garcia
left a comment
There was a problem hiding this comment.
This lgtm but I believe we should circle other folks for thoughts before merging.
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/sentry/develop/3EmqYRmvNBn2Z3tvP7EFPZhkfZrm |
We've established that incoming requests have op=http.server and outgoing requests have op=http.client. Specification available at: https://develop.sentry.dev/sdk/features/#http-client-integrations Last update: getsentry/develop#341
We've established that incoming requests have op=http.server and outgoing requests have op=http.client. Specification available at: https://develop.sentry.dev/sdk/features/#http-client-integrations Last update: getsentry/develop#341
5 participants
Fixes #254.