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

Add OkHttp event spans #2659

Merged
merged 14 commits into from
May 26, 2023
Merged

Add OkHttp event spans #2659

merged 14 commits into from
May 26, 2023

Conversation

stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Apr 17, 2023

📜 Description

added SentryOkHttpEventListener that creates a span on every OkHttp event
added SentryOkHttpEvent

Here is an example of the result.
Screenshot 2023-04-21 at 09 44 35

💡 Motivation and Context

We are increasing the information shown to the user by adding spans for OkHttp events, including dns, https setup, proxies, request headers/body and response headers/body

💚 How did you test it?

Unit test

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Update docs https://docs.sentry.io/platforms/android/configuration/integrations/okhttp/

@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 247ceb3

@stefanosiano
Copy link
Member Author

I just need a confirmation if some data could be seen as PII

@stefanosiano stefanosiano marked this pull request as ready for review April 17, 2023 10:10
@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 303.78 ms 336.69 ms 32.91 ms
Size 1.72 MiB 2.28 MiB 570.91 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
46b1782 387.72 ms 458.74 ms 71.02 ms
1707044 338.80 ms 384.79 ms 46.00 ms

App size

Revision Plain With Sentry Diff
46b1782 1.72 MiB 2.28 MiB 570.44 KiB
1707044 1.72 MiB 2.28 MiB 570.44 KiB

Previous results on branch: feat/okhttp-event-spans

Startup times

Revision Plain With Sentry Diff
6244bee 244.04 ms 390.08 ms 146.04 ms
80dcf36 311.78 ms 350.02 ms 38.24 ms
418cd6d 339.98 ms 381.98 ms 42.00 ms
19b506b 386.54 ms 413.02 ms 26.48 ms
4e5eb0c 357.83 ms 363.70 ms 5.87 ms
f79cd6a 295.88 ms 358.24 ms 62.36 ms
f08b219 320.40 ms 372.40 ms 52.00 ms

App size

Revision Plain With Sentry Diff
6244bee 1.73 MiB 2.26 MiB 550.35 KiB
80dcf36 1.72 MiB 2.28 MiB 570.67 KiB
418cd6d 1.72 MiB 2.28 MiB 570.44 KiB
19b506b 1.72 MiB 2.28 MiB 565.45 KiB
4e5eb0c 1.73 MiB 2.26 MiB 550.51 KiB
f79cd6a 1.72 MiB 2.28 MiB 570.44 KiB
f08b219 1.73 MiB 2.26 MiB 550.38 KiB

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.05 🎉

Comparison is base (ba90bd9) 81.05% compared to head (247ceb3) 81.11%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2659      +/-   ##
============================================
+ Coverage     81.05%   81.11%   +0.05%     
- Complexity     4427     4449      +22     
============================================
  Files           345      345              
  Lines         16356    16407      +51     
  Branches       2219     2226       +7     
============================================
+ Hits          13258    13308      +50     
- Misses         2170     2172       +2     
+ Partials        928      927       -1     

see 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@marandaneto
Copy link
Contributor

@stefanosiano should we close this then? #2158

@marandaneto
Copy link
Contributor

@stefanosiano can you attach a screenshot of the result?

@stefanosiano
Copy link
Member Author

stefanosiano commented Apr 21, 2023

@stefanosiano should we close this then? #2158

I think so. I wanted to use and expand that, but there were several differences, so I ended up creating a whole new pr

@stefanosiano can you attach a screenshot of the result?

Added it in the description

@marandaneto marandaneto mentioned this pull request Apr 21, 2023
4 tasks
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

After a quick glance looks good, but I'll defer to @markushi for proper review

SentryOkHttpEventListener now accepts an instance of EventListener or EventListener.Factory and propagate all the calls to it
@stefanosiano
Copy link
Member Author

Let's update the span data keys to align with https://develop.sentry.dev/sdk/performance/span-data-conventions/

@marandaneto
Copy link
Contributor

@stefanosiano can we actually add the children's span nested to its parents?
For example, secureConnect is a child of connect, etc.
Since you have the reference of each of them, it's possible, I recall @bruno-garcia asking this as well when I did the first PoC.

@stefanosiano
Copy link
Member Author

@stefanosiano can we actually add the children's span nested to its parents? For example, secureConnect is a child of connect, etc. Since you have the reference of each of them, it's possible, I recall @bruno-garcia asking this as well when I did the first PoC.

Sure, i'll add it

@stefanosiano stefanosiano marked this pull request as draft May 2, 2023 08:09
Removed `http.` prefix from the breadcrumb
Updated sub-spans with operation from `http.client` to `http.client.details`
removed filtered_url from SentryOkHttpEvent
Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

@stefanosiano I've left a few names, there's a spec for crumbs and spans, let's use the very same keys as per spec.

removed `success` from http span as it's implied by the http.status_code
http sub-spans operation now contains the event, and the description is now empty
@stefanosiano
Copy link
Member Author

@marandaneto would you take another look?

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

Left a few more comments, if the maintainers are happy, I am happy.

@marandaneto marandaneto self-requested a review May 25, 2023 07:37
@markushi
Copy link
Member

@marandaneto could you unblock this PR - we want to merge and ship this today 😊

@marandaneto
Copy link
Contributor

happy

It's not blocked, #2659 (review)
Only one approval is needed.

@romtsn romtsn requested review from marandaneto and removed request for marandaneto May 26, 2023 08:52
@romtsn
Copy link
Member

romtsn commented May 26, 2023

happy

It's not blocked, #2659 (review) Only one approval is needed.

I think you have to approve it actually
image

@stefanosiano stefanosiano enabled auto-merge (squash) May 26, 2023 09:29
@markushi markushi dismissed marandaneto’s stale review May 26, 2023 09:33

if the maintainers are happy, I am happy.

@stefanosiano stefanosiano merged commit fe90ed9 into main May 26, 2023
@stefanosiano stefanosiano deleted the feat/okhttp-event-spans branch May 26, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants