getsentry / sentry-java

A Sentry SDK for Java, Android and other JVM languages.
https://docs.sentry.io/
MIT License
1.14k stars 432 forks source link

`SentryEvent#getExtras` is package-private. #1383

Open wzieba opened 3 years ago

wzieba commented 3 years ago

Platform:

IDE:

Build system:

Android Gradle Plugin:

Sentry Android Gradle Plugin:

Proguard/R8:

Platform installed with:

The version of the SDK: 4.3.0


I have the following issue:

I'm writing a library that proxies Sentry. One of the use cases we have is to provide a callback to the library client before sending a SentryEvent. In this callback we'd like to attach all SentryEvent's extras so the client can decide what to do: add some new extras, update or existing ones.

As Sentry doesn't allow us to get all extras (only a single extra for key), I can't see a way to implement our use case.

Would you consider making SentryEvent#getExtras a public method?

Actual result:

Expected result:

marandaneto commented 3 years ago

@wzieba thanks for raising this, it was public previously, now it's been private due to other concerns, I'm not entirely sure if we'd like to do that, @bruno-garcia opinions?

there are ways around that, but obviously suboptimal and not recommended.

marandaneto commented 3 years ago

people could always do getExtra(x), removeExtra(x)or setExtras(null), setExtra(x, y), so if they know what they want to add/delete, its pretty much possible with the current public methods

wzieba commented 3 years ago

people could always do getExtra(x), removeExtra(x)or setExtras(null), setExtra(x, y), so if they know what they want to add/delete

Right, it's possible but I'd like to have API for the library to show all existing extra keys in callback, so client can decide what it wants to add or not, based on existing keys. Now I have to inform library about extra keys I want to check/change beforehand.

It's not a blocker for us - it's more like inconvenience.

maciejwalkowiak commented 3 years ago

The alternative would be to make getExtras public and return a copy of the map, but same pattern would have to be applied in other places in the SDK too.

bruno-garcia commented 3 years ago

What about we just make this public though?

I believe the reason might have been because Extra is "soft-deprecated". But it's still rather useful particularly when you really don't have a "key" for a whole group of information in order to use Contexts instead. Sometimes you really only have a single key-value pair and today Extra is still the way to go.

marandaneto commented 3 years ago

@bruno-garcia that was not the reason, but because the public API have setWhatever(x, y) and removeWhatever(x), this is not only about Extra but also for Tags etc. I was all in to keep them public, but we had to remove it, so unless I understand why, I'd keep it as it is.

bruno-garcia commented 3 years ago

I don't recall why we made it package private but it's pretty standard in Sentry SDKs that the scope is 'write only'. It doesn't come with accessors for the fields there so I'm also happy to keep it as it is:

@bruno-garcia that was not the reason, but because the public API have setWhatever(x, y) and removeWhatever(x), this is not only about Extra but also for Tags etc. I was all in to keep them public, but we had to remove it, so unless I understand why, I'd keep it as it is.

Sorry I don't follow. We made the getter package private because of what exactly? I understand we have setExtra(x,y) and remove, and that's the same for tags. I also understand the unified API doesn't include any getters. So if it was to align with that, it would also explain. Because I don't recall the exact reason I suggested it could have been due to the soft deprecation but I'm really just guessing.

marandaneto commented 3 years ago

we made package-private because our public API already exposes a setter with key/value and a remove method with key, for every Map, so we don't expose the field with a getter.

@maciejwalkowiak option of exposing the getter but returning an immutable copy was an option at that time, but we preferred to follow the other SDKs which is not exposing at all, I like this idea to be honest

bruno-garcia commented 3 years ago

so we don't expose the field with a getter.

That makes sense. I was under the impression it was a getter for a key, not for the whole map. getExtra("key").

Came up again on a meeting today of how Scope is ready-only. So we might even get to a point where we have to remove all getters.