Merging of undefined external option of type "list" overrides internal one actually defined

[dalbani]

Integration

sentry

Java Version

21

Version

8.2.0

Steps to Reproduce

Create a SentryHandler like the following, in this case setting the list of ignored errors:

SentryOptions options = new SentryOptions();
options.setIgnoredErrors(List.of("error1", "error2"));
...
SentryHandler handler = new SentryHandler(options);
...

Register the handler and when an issue is handled by Sentry, realise that the list of ignored errors is actually empty.

This is due to the fact that:

1. The SentryHandler constructor always sets enableExternalConfiguration = true and thus triggers a configuration merge
2. PropertiesProvider::getList() returns an empty list if the property (e.g. ignored-errors in this case) is missing
3. SentryOptions::setIgnoredErrors() called by the merge() function replaces the list containing [error1, error2] from the internal options with an empty list [] created previously

Could you provide a way to instantiate a SentryHandler which doesn't enable external configuration?

Expected Result

The list of ignored errors is [error1, error2].

Actual Result

The list of ignored errors is [] (empty list).

[adinauer]

Thanks for opening this issue @dalbani looks like that's a bug in the SDK.

As a workaround, can you add the errors you want to ignore using e.g. sentry.properties or SENTRY_IGNORED_ERRORS env var?

To solve this, I suggest we fix it by either

1. reading null instead of an empty list in ExternalOptions (through something like propertiesProvider.getListOrNull)
2. making SentryOptions.merge add the list of entries to its existing entries instead of overwriting
   • this would require a major release IMO
   • this would no longer allow deliberate overwriting of values using external config (like sentry.properties and ENV vars)
3. making external config optional in all integrations that set it
   • this would not be possible in all integrations since some of them are initialized through some hook instead of manually instantiated in code

I like 1. best here since 2 would require a major and implementing 3. only for SentryHandler would only partially fix this problem / require more complex changes for some integrations. 1. should fix the problem for everyone without having to learn about how to disable external config and then losing out on functionality.

We'll try to include a fix in the next release.

[dalbani]

Thanks for your quick response!

As a workaround, can you add the errors you want to ignore using e.g. sentry.properties or SENTRY_IGNORED_ERRORS env var?

Yes, that's exactly what I did to work around the issue.

Regarding the different solutions, I was wondering if the SDK could provide a constructor for SentryHandler that doesn't set enableExternalConfiguration = true?
An example of such a use case would be integrations like https://github.com/quarkiverse/quarkus-logging-sentry/, which have their own way of configuring Sentry, and where one may not want to have external factors (e.g. SENTRY_* env variables) affect the final Sentry configuration.

[adinauer]

@dalbani I'm not opposed to adding a new constructor for SentryHandler, I just don't think it's the right fix for this bug (which has a PR open to fix it as described in 1.).

We can also add the constructor if you need it.

[dalbani]

Indeed, the fix in 1. is the proper solution for the issue that I raised.

I (mis)used the opportunity of this discussion to make my request for a new constructor 😄
Shall I register a new issue (feature request) for more clarity?

[adinauer]

Nah, let's keep it simple, I opened a PR: #4208

[dalbani]

Great, thanks a lot!