Skip to content
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

fix: Update span op for outgoing HTTP requests #3821

Merged
merged 2 commits into from
Jul 20, 2021

Conversation

rhcarvalho
Copy link
Contributor

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


Note: this has the unfortunate potential to break anything relying on the old op value. While the change is simple, some consideration needs to be taken.

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
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

This will have product ramifications. Pinging @getsentry/visibility to get their thoughts.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 21.46 KB (0%)
@sentry/browser - Webpack 22.47 KB (0%)
@sentry/react - Webpack 22.5 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.97 KB (+0.02% 🔺)

@kamilogorek
Copy link
Contributor

Nice, our tests caught something :D

@rhcarvalho
Copy link
Contributor Author

Nice, our tests caught something :D

Haha, yeah, it was a .js file (nextjs integration tests), excluded from my search filter :)

@k-fish
Copy link
Member

k-fish commented Jul 20, 2021

This will have product ramifications. Pinging @getsentry/visibility to get their thoughts.

Do you mean operations breakdown?

Ops breakdown uses http with startsWith in relay so it shouldn't be affect if we are just adding more to the op.

http
https://github.com/getsentry/sentry/blob/809b7fe54c6f06cc1e4c503cf83ded896472a011/src/sentry/projectoptions/defaults.py#L74

startsWith
https://github.com/getsentry/relay/blob/9fd1a4310ea7f18a09df9f3a772339a546d6e422/relay-general/src/store/normalize/breakdowns.rs#L148

@dashed
Copy link
Member

dashed commented Jul 20, 2021

@k-fish @rhcarvalho @AbhiPrasad tldr: this has no product ramifications; this rename will match cleanly on the operation names breakdown feature. In other words, http.client and http.server will group to http spans on the product.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Perfect, think we are good to go then.

@rhcarvalho rhcarvalho merged commit 316566e into master Jul 20, 2021
@rhcarvalho rhcarvalho deleted the rhcarvalho/http.client branch July 20, 2021 19:44
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.

None yet

5 participants