sentry-java icon indicating copy to clipboard operation
sentry-java copied to clipboard

Align ignoredExceptionsForType naming on Java and .NET

Open marandaneto opened this issue 3 years ago • 8 comments

On Java, its addIgnoredExceptionForType(x) on .NET AddExceptionFilterForType(x)

https://docs.sentry.io/platforms/dotnet/usage/#ignoring-errors

marandaneto avatar Jun 15 '21 07:06 marandaneto

I like the AddExceptionFilterForType

lucas-zimerman avatar Jan 19 '22 13:01 lucas-zimerman

I agree with the motivation behind aligning names of options and other things across SDKs. We should have that as a goal for anything we do.

With regards of changing these methods, I find the drawback of introducing a breakage on our users for the sake of 'aligning the name with .NET" it's not really worth it. If we add the second name with a Obsolete annotation on it indicating the users should use the new name, meaning no breaking code change, I'm more in favor of it but just "deleting" the method and writing the new one on the Migration Guide is too much friction and not worth the benefit.

Worth noting if the docs don't have the right name, we should amend that.

bruno-garcia avatar Apr 27 '22 14:04 bruno-garcia

So Java is already documented correctly https://docs.sentry.io/platforms/java/configuration/options/#ignored-exceptions-for-type

Usually, we don't do breaking changes before annotating the method with @Deprecated and @ScheduledForRemoval so folks have time to migrate before getting a compile error. I'm fine either way, I'm not a big fan of special case naming per platform because the docs become a little bit of a nightmare to be maintained because of all duplications, that's why we have a Unified API as well.

Also, Usually, it's a single person answering all the questions from multiple sources about "why this does not exist?" and you have to dig into the source code to know where and why it's under a different name for any reason, sometimes even because its a reserved word, a simple and fresh example https://github.com/getsentry/sentry-react-native/issues/2213

That said, both docs are up to date with the naming, we can decide to close this issue if you are all fine with it, but I vote for aligning them, this is not a priority though, that's why the milestone v6 has been removed on the 10th of March, so it's not a blocker for the Java release v6.

marandaneto avatar Apr 27 '22 15:04 marandaneto

Suggested course of action: Deprecate existing option and add new option Then I guess for 7.0 we can remove the deprecated option.

adinauer avatar Apr 27 '22 15:04 adinauer

Suggested course of action: Deprecate existing option and add new option Then I guess for 7.0 we can remove the deprecated option.

Yes, that's what we always did, example https://github.com/getsentry/sentry-java/pull/1873/files

marandaneto avatar Apr 27 '22 15:04 marandaneto

After looking the the .NET code here https://github.com/getsentry/sentry-dotnet/blob/720e8d1aa75f1603f7712242bb3fcf365917ee19/src/Sentry/SentryOptionsExtensions.cs#L116 and here https://github.com/getsentry/sentry-dotnet/blob/720e8d1aa75f1603f7712242bb3fcf365917ee19/src/Sentry/Extensibility/IExceptionFilter.cs this is not just a naming difference but also differs in how this is implemented.

Java uses a simple list of classes to be ignored whereas .NET allows for more complex filters as well (https://github.com/getsentry/sentry-dotnet/blob/720e8d1aa75f1603f7712242bb3fcf365917ee19/src/Sentry/SentryOptionsExtensions.cs#L106).

Feels like the current naming in Java fits its implementation quite well, so how about just adding addExceptionFilterForType without deprecating the existing addIgnoredExceptionForType? Then we support the common name from other SDKs and can document it.

adinauer avatar Apr 29 '22 08:04 adinauer

@adinauer for me, they look exactly the same (the issue is about the ignoredExceptionsForType method), See code snippet https://docs.sentry.io/platforms/dotnet/usage/#ignoring-errors https://docs.sentry.io/platforms/java/configuration/#ignored-exceptions-for-type

marandaneto avatar Apr 29 '22 08:04 marandaneto

Yes the way to add it is the same because .NET has this method to make it easier to add a filter for a type without having to create the filter yourself. My point is that renaming everything in the Java SDK doesn't seem right because it's not really a list of filters but just a list of classes. To me that would make the naming misleading.

So my new suggestion is to just add the method to align SDKs regarding how to add an Exception that should be ignored but keep the old naming in place and also keep all the internal naming for this.

adinauer avatar Apr 29 '22 09:04 adinauer