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

Implement local scope by adding overloads to the capture methods that accept a ScopeCallback #2084

Merged
merged 10 commits into from
Jun 9, 2022

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Jun 7, 2022

📜 Description

Reopened because #2075 was auto closed by GitHub ...

Add overloads with scope callback for captureEvent, captureException and captureMessage to allow users to modify the scope for a single invocation of the capture methods.

💡 Motivation and Context

fixes #1829

💚 How did you test it?

📝 Checklist

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

🔮 Update Docs

@adinauer adinauer changed the title Feature/local scope Implement local scope by adding overloads to the capture methods that accept a ScopeCallback Jun 7, 2022
CHANGELOG.md Outdated

### Features

- Implement local scope by adding overloads to the capture methods that accept a ScopeCallback ([#2075](https://github.com/getsentry/sentry-java/pull/2075))
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
- Implement local scope by adding overloads to the capture methods that accept a ScopeCallback ([#2075](https://github.com/getsentry/sentry-java/pull/2075))
- Implement local scope by adding overloads to the capture methods that accept a ScopeCallback ([#2084](https://github.com/getsentry/sentry-java/pull/2084))

@adinauer
Copy link
Member Author

adinauer commented Jun 7, 2022

What do we want to do with the docs here https://docs.sentry.io/platforms/java/enriching-events/scopes/#local-scopes

The docs claim that withScope does what this does. Which is not true as the scope is only reset after withScope completes. Should we replace the docs for "Local Scope" and only mention the new overloads from this PR?

@adinauer
Copy link
Member Author

adinauer commented Jun 7, 2022

Opened #2083 to deprecate withScope.

@adinauer adinauer self-assigned this Jun 7, 2022
CHANGELOG.md Outdated
## 6.0.0

### Sentry Self-hosted Compatibility

- Starting with version `6.0.0` of the `sentry` package, [Sentry's self hosted version >= v21.9.0](https://github.com/getsentry/self-hosted/releases) is required or you have to manually disable sending client reports via the `sendClientReports` option. This only applies to self-hosted Sentry. If you are using [sentry.io](https://sentry.io), no action is needed.

### Features

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather keep a line break after the headers

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, wasn't intentional :) new line is back

Copy link
Member Author

@adinauer adinauer Jun 8, 2022

Choose a reason for hiding this comment

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

I think there were 2 line breaks before all the merging happened.

private Scope buildLocalScope(
final @NotNull Scope scope, final @Nullable ScopeCallback callback) {
if (callback != null) {
Scope localScope = new Scope(scope);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Scope localScope = new Scope(scope);
final Scope localScope = new Scope(scope);

@marandaneto
Copy link
Contributor

No breaking changes

It's technically a breaking change since it adds new methods to the public interfaces e.g. IHub and they are not defined as default, so people implementing their own Hub would need to add such new methods.

@marandaneto
Copy link
Contributor

After releasing this, we can finally add local scope support for our Android support as well, this would require changing the docs for Java and Android, https://docs.sentry.io/platforms/android/enriching-events/scopes/ and https://docs.sentry.io/platforms/java/enriching-events/scopes/#local-scopes

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.

@adinauer Thanks for doing this.
Left a few comments, but LGTM anyway so you are unblocked to fix and merge it.

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2022

Codecov Report

Merging #2084 (10be04d) into main (c62a9f7) will decrease coverage by 0.07%.
The diff coverage is 58.82%.

@@             Coverage Diff              @@
##               main    #2084      +/-   ##
============================================
- Coverage     81.18%   81.11%   -0.08%     
- Complexity     3223     3232       +9     
============================================
  Files           230      230              
  Lines         11821    11850      +29     
  Branches       1572     1572              
============================================
+ Hits           9597     9612      +15     
- Misses         1638     1652      +14     
  Partials        586      586              
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/HubAdapter.java 7.81% <0.00%> (-0.39%) ⬇️
sentry/src/main/java/io/sentry/NoOpHub.java 45.45% <0.00%> (-3.33%) ⬇️
sentry/src/main/java/io/sentry/Sentry.java 45.56% <0.00%> (-1.68%) ⬇️
sentry/src/main/java/io/sentry/IHub.java 81.48% <60.00%> (-6.02%) ⬇️
sentry/src/main/java/io/sentry/Hub.java 76.41% <100.00%> (+0.87%) ⬆️

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 c62a9f7...10be04d. Read the comment docs.

@adinauer
Copy link
Member Author

adinauer commented Jun 8, 2022

@adinauer Thanks for doing this.

@lbloder created the PR, i just reopened after GitHub auto closed it. So thanks goes to him :-)

@adinauer
Copy link
Member Author

adinauer commented Jun 9, 2022

Next step: add docs as suggested and keep withScope for now until it's deprecated.

After releasing this, we can finally add local scope support for our Android support as well, this would require changing the docs for Java and Android, https://docs.sentry.io/platforms/android/enriching-events/scopes/ and https://docs.sentry.io/platforms/java/enriching-events/scopes/#local-scopes

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.

Implement local scope, which is a method parameter for the capture methods
4 participants