Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

adding scope methods to sentry static class #179

Merged
merged 5 commits into from Dec 3, 2019

Conversation

marandaneto
Copy link
Contributor

馃摙 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

馃摐 Description

adding scope methods to sentry static class

馃挕 Motivation and Context

making migration easier, otherwise one will need to call Sentry.configureScope(...)

馃挌 How did you test it?

I'll do unit tests

馃摑 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing

馃敭 Next steps

@codecov-io
Copy link

codecov-io commented Dec 3, 2019

Codecov Report

Merging #179 into master will increase coverage by 0.31%.
The diff coverage is 55.78%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #179      +/-   ##
============================================
+ Coverage      57.8%   58.11%   +0.31%     
- Complexity      532      552      +20     
============================================
  Files            73       73              
  Lines          2529     2617      +88     
  Branches        223      233      +10     
============================================
+ Hits           1462     1521      +59     
- Misses          958      978      +20     
- Partials        109      118       +9
Impacted Files Coverage 螖 Complexity 螖
sentry-core/src/main/java/io/sentry/core/IHub.java 100% <酶> (酶) 4 <0> (酶) 猬囷笍
.../src/main/java/io/sentry/core/cache/DiskCache.java 53.03% <酶> (酶) 12 <0> (酶) 猬囷笍
.../java/io/sentry/core/transport/NoOpEventCache.java 0% <酶> (酶) 0 <0> (酶) 猬囷笍
...ntry/core/UncaughtExceptionHandlerIntegration.java 77.04% <酶> (酶) 9 <0> (酶) 猬囷笍
...ntry-core/src/main/java/io/sentry/core/Sentry.java 0% <0%> (酶) 0 <0> (酶) 猬囷笍
...try-core/src/main/java/io/sentry/core/NoOpHub.java 69.56% <0%> (-30.44%) 16 <0> (酶)
...entry-core/src/main/java/io/sentry/core/Scope.java 87.95% <100%> (+2.4%) 28 <1> (+1) 猬嗭笍
sentry-core/src/main/java/io/sentry/core/Hub.java 67.47% <72.05%> (+1.25%) 55 <19> (+17) 猬嗭笍
...rc/main/java/io/sentry/core/CircularFifoQueue.java 37.39% <0%> (+4.06%) 18% <0%> (+1%) 猬嗭笍
...in/java/io/sentry/core/SynchronizedCollection.java 35.29% <0%> (+7.84%) 8% <0%> (+1%) 猬嗭笍

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 cee0f0a...7dabcdb. Read the comment docs.

@bruno-garcia
Copy link
Member

I'd like to record my thinking here for @fdrabek and @HazAT .
Adding these method to Sentry. is not ideal. The reason is, now there's even more ways to do simple stuff, but this is more unclear how it works.

Sentry.setLevel();
Sentry.pushScope();
Sentry.setLevel();

It looks even more like magic. What's happening under the hood that the ambient data propagates down?

Also, not all methods are on Sentry. which means we still need configureScope. This makes it extra confusing. If we're going this direction, I'd say we add everything to Sentry. and dump configureScope all together (which is not my favorite option anyway).

The argument to do this in JS was that passing a cloujure was "too advance" for our users. That one never made much sense.
But the real motive was to make it easier to port from old users. One on in Java has requested that yet. We could simply provide a migration guide.

There was no pushScope. There's anyway a new concept. We need to introduce users to the new concept anyway.

Instead of poluting the API I believe we should figure out a way to get this to work with async frameworks because Sentry.setLevel will not work at all. Neither is our current scope management based on thread-local-storage.

@HazAT
Copy link
Member

HazAT commented Dec 3, 2019

@bruno-garcia

But the real motive was to make it easier to port from old users. One on in Java has requested that yet. We could simply provide a migration guide.

True, no one requested it yet but the number of people potentially not updating because they can't simply string replace

eventBuilder.withExtra(entry.getKey(), entry.getValue());
// with
Sentry.setExtra(entry.getKey(), entry.getValue());

is something that's hard to measure. Just one person writing could mean, a 100 users didn't even bother to write an issue and another 1000 users complety discarded the idea of upgrading.

I agree with your side of adding more API is generally always bad I think we need to take the hit in order to make it simpler, even it's just for a few users.

Also, those functions were so people don't need to even understand what a scope is. We don't want to force anyone learning how powerful a scope can be, if they have a problem they can't solve with a simple setTag, then it's time they move to learn about scopes and not earlier.

All things aside, I think it would make sense to have a general discussion about the unified API, specifically talking about what we've learned in the last 1-2 years about it. It's for sure we would make some things a bit different.

@marandaneto marandaneto merged commit 2b51fbd into master Dec 3, 2019
@marandaneto marandaneto deleted the enha/sentry_sets_scope branch December 3, 2019 15:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants