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 coroutines support #1479

Merged
merged 14 commits into from
May 26, 2021
Merged

Add coroutines support #1479

merged 14 commits into from
May 26, 2021

Conversation

maciejwalkowiak
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak commented May 18, 2021

📜 Description

Adds the ability to use Sentry scope with Kotlin coroutines through a coroutines context element SentryContext.

💡 Motivation and Context

Fixes #973.

💚 How did you test it?

Unit tests.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

It would be nice if someone with coroutines experience review it.

@maciejwalkowiak maciejwalkowiak changed the title First attempt to make SDK coroutines-friendly. Add coroutines support May 24, 2021
@maciejwalkowiak maciejwalkowiak marked this pull request as ready for review May 24, 2021 13:33
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2021

Codecov Report

Merging #1479 (f776e9a) into main (ff30354) will decrease coverage by 0.00%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1479      +/-   ##
============================================
- Coverage     75.66%   75.65%   -0.01%     
- Complexity     1926     1929       +3     
============================================
  Files           190      191       +1     
  Lines          6590     6598       +8     
  Branches        666      666              
============================================
+ Hits           4986     4992       +6     
- Misses         1278     1280       +2     
  Partials        326      326              
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/Sentry.java 42.65% <0.00%> (-0.61%) ⬇️
...ns/src/main/java/io/sentry/kotlin/SentryContext.kt 100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/Scope.java 93.15% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff30354...f776e9a. Read the comment docs.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

From my perspective LGTM!

We could get feedback from folks that have been interested in this before shipping though.
And validate that question whether it's OK to have 1 package with ktx + the coroutines, or if it's going to be a problem for too many ppl

sentry-kotlin-extensions/build.gradle.kts Show resolved Hide resolved
sentry/src/main/java/io/sentry/Sentry.java Show resolved Hide resolved
@marandaneto
Copy link
Contributor

@maciejwalkowiak I've created the docs issue as well getsentry/sentry-docs#3628

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.

nice, thanks @maciejwalkowiak
LGTM

@maciejwalkowiak maciejwalkowiak merged commit b6d1c5b into main May 26, 2021
@maciejwalkowiak maciejwalkowiak deleted the gh-973 branch May 26, 2021 08:37
@iamkdblue
Copy link

I am trying to add support for sentry coroutine in the Spring Boot project given
Here

i am using implementation "io.sentry:sentry-kotlin-extensions:5.0.0"

but its throwing error

Execution failed for task ':controller:compileKotlin'.
> Could not resolve all files for configuration ':controller:compileClasspath'.
   > Could not find io.sentry:sentry-kotlin-extensions:5.0.0.
     Searched in the following locations:
       - https://repo.maven.apache.org/maven2/io/sentry/sentry-kotlin-extensions/5.0.0/sentry-kotlin-extensions-5.0.0.pom
       - https://jcenter.bintray.com/io/sentry/sentry-kotlin-extensions/5.0.0/sentry-kotlin-extensions-5.0.0.pom
     Required by:
         project :controller

Possible solution:
 - Declare repository providing the artifact, see the documentation at https://docs.
```gradle.org/current/userguide/declaring_repositories.html

@marandaneto
Copy link
Contributor

@iamkdblue

Support for Kotlin Coroutines is available as a Beta. Features available in Beta are still in-progress and may have bugs. We recognize the irony.

please use this version for now https://github.com/getsentry/sentry-java/releases/tag/5.0.0-beta.7, 5.0.0 isn't out yet

@iamkdblue
Copy link

iamkdblue commented Jun 1, 2021

@marandaneto

cant find SentryContext() and launch in spring boot
and i am using withContext(Dispatchers.IO) not launch, mostly launch(fire and forget) uses in android!

@marandaneto
Copy link
Contributor

@iamkdblue please raise a new issue with your setup and what you're trying to do, it'd be easier to track it down and fix it if there are issues, thanks

@markallanson
Copy link

markallanson commented Jun 22, 2021

Tested this out in 5.0.1 but it doesn't seem to work. Using this with the grpc-kotlin support and providing a SentryContext to the CoroutineScope it creates, but when I am inside coroutines the Hub seems to resolve to the NoOpHub hence sentry errors are not reported anywhere.

Using sentry-spring-boot-starter to initialize Sentry, and outside the coroutines the Hub is resolved correctly.

@marandaneto
Copy link
Contributor

@markallanson please raise a new issue with more detailed context, happy to look into it, thanks

@markallanson
Copy link

I got to the bottom of this, the sentry-spring-boot-starter wasn't initialising sentry before the SentryContext was created causing it to use the NoOpHub.

@marandaneto
Copy link
Contributor

@markallanson so was it a bug in your end or what, does it mean you're creating the SentryContext before even initing Sentry?

@markallanson
Copy link

@marandaneto I need to try and get the sentry-spring-boot-starter to initialize sentry before my services try and use the context.

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.

Kotlin Coroutine Support
6 participants