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

Failure Analysis for InvalidConfigurationPropertyValueException is skipped when the property is not set #33261

Closed
snicoll opened this issue Nov 18, 2022 · 5 comments
Labels
type: bug A general bug
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Nov 18, 2022

There are a number of places where we throw InvalidConfigurationPropertyValueException to indicate that a value should have been set. WavefrontProperties does so if the API token has not been set.

The related InvalidConfigurationPropertyValueFailureAnalyzer has no effect as it tries to find a property in the environment that matches the one in the exception. If no property is set, the analysis doesn't do anything and the raw exception is thrown.

I don't know what's the best course of action for this is. Invalid and missing are two different things and I am not sure the failure analyzer should be the one deciding which is which.

@snicoll snicoll added the type: bug A general bug label Nov 18, 2022
@snicoll snicoll added this to the 2.6.x milestone Nov 18, 2022
@wilkinsona wilkinsona modified the milestones: 2.6.x, 2.7.x Nov 24, 2022
@protyay
Copy link
Contributor

protyay commented Nov 25, 2022

Hey, is anyone looking into this ? If there's not a too strict timeline, I can dive deep. Let me know what's your opinion

@wilkinsona
Copy link
Member

Thanks for the offer, @protyay. No one is looking at it and you're more than welcome to do so, but we really don't know how to tackle this one yet. As @snicoll said, the best course of action isn't clear so we probably need to spend some time figuring out what needs to be done before anyone can start working on this one. I'll update the issue's labels to reflect this.

@wilkinsona wilkinsona added the status: pending-design-work Needs design work before any code can be developed label Nov 25, 2022
@philwebb philwebb modified the milestones: 2.7.x, 3.1.x Nov 8, 2023
@wilkinsona
Copy link
Member

We seem to have two places where we create an InvalidConfigurationPropertyValueException with a null value:

if (this.apiTokenType != TokenType.NO_TOKEN && this.apiToken == null && !usesProxy()) {
throw new InvalidConfigurationPropertyValueException("management.wavefront.api-token", null,
"This property is mandatory whenever publishing directly to the Wavefront API");
}

if (this.properties.getStreams().getApplicationId() == null) {
String applicationName = environment.getProperty("spring.application.name");
if (applicationName == null) {
throw new InvalidConfigurationPropertyValueException("spring.kafka.streams.application-id", null,
"This property is mandatory and fallback 'spring.application.name' is not set either.");
}

In each case, I don't think we're really throwing the right exception.

In the WavefrontProperties case, it's not one individual property that has an invalid value but a combination of two properties. management.wavefront.api-token-type has been set but management.wavefront.api-token has not and setting the former to a value other than no-token requires that api-token is also set. There are two ways to fix this:

  • Correct the value of api-token-type and set it to no-token
  • Set api-token with a token of the configured type

In the Kafka Streams case, it's two properties again as neither spring.application.name nor spring.kafka.streams.application-id has been set. Setting either of them will fix the problem so it's not necessarily the value of spring.kafka.streams.application-id that's invalid.

We could update InvalidConfigurationPropertyValueFailureAnalyzer to perform analysis when the property cannot be found in the environment (for example by using the value from the InvalidConfigurationPropertyValueException and omitting the origin information). I think we could also introduce new exceptions for the two cases above:

  1. Some configuration properties are invalid in combination
  2. One or more mandatory properties has not been set

For 2, we should perhaps also consider making the auto-configuration back off somehow rather than failing when the property isn't set. I'm not sure if that would be the best behavior here but the way it currently works does feel a little unusual.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Apr 24, 2024
@wilkinsona
Copy link
Member

With some changes in place to still perform analysis when the environment doesn't contain the property, something like this is logged:

***************************
APPLICATION FAILED TO START
***************************

Description:

Invalid value 'null' for configuration property 'management.wavefront.api-token'. Validation failed for the following reason:

This property is mandatory whenever publishing directly to the Wavefront API

Action:

Review the value of the property with the provided reason.

For this particular case it could be improved with a custom exception for an invalid combination of property values but I think it's better than no analysis at all for the general null value case.

@philwebb philwebb removed status: pending-design-work Needs design work before any code can be developed for: team-meeting An issue we'd like to discuss as a team to make progress labels May 9, 2024
@philwebb
Copy link
Member

philwebb commented May 9, 2024

We're going to add the fix Andy suggested above and look at adding a different exception type for Wavefront and Kafka.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants