SWATCH-1801: Use simple solution to replace SPeL expression parser #2611
Conversation
Sgitario
force-pushed
the
refactor_query_builder
branch
from
42c5324
to
7338c50
Compare
| import org.springframework.stereotype.Component; | ||
|
|
||
| /** Builds PromQL queries based on a configured template. */ | ||
| @Component | ||
| public class QueryBuilder { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(QueryBuilder.class); | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(QueryBuilder.class); |
There was a problem hiding this comment.
Please just use log. That's what Lombok generates and I'd like to remain consistent across the codebase. In fact, just dropping an @Slf4j annotation on this class seems like the best thing to do. We haven't gone back and done that everywhere (you'll see it used some but not consistently) but it seems like the most sensible thing to do going forward.
| String defaultValue = null; | ||
| if (rawKey.contains(SYSTEM_PROPERTY_DEFAULT_TAG)) { | ||
| String[] keyParts = rawKey.split(SYSTEM_PROPERTY_DEFAULT_TAG); | ||
| key = keyParts[0]; |
There was a problem hiding this comment.
I would like to do some checks here. Specifically, require key to begin with an metric prefix (and maybe need to add runtime as well?). Giving this class unfettered access to the system properties and environment variables can have security implications. I think it would be safer to just eliminate access to any property or environment variable that doesn't start with a blessed prefix.
There was a problem hiding this comment.
Let's agree first if this is the right approach or not. I'm in favor of getting rid of the system properties usage altogether.
We have a meeting today to discuss the approach to use to replace the SPeL expression parser.
There was a problem hiding this comment.
After speaking with the team, we agreed to use this approach. I've already removed the support of system properties.
Sgitario
force-pushed
the
refactor_query_builder
branch
from
7338c50
to
bf610b7
Compare
Sgitario
force-pushed
the
refactor_query_builder
branch
from
7680df4
to
04663d2
Compare
Jira issue: SWATCH-1801
Description
This is the simplest solution I can think of, to replace the SPeL expression parser which perfectly works for our use case and avoids the usage of reflection (so it's compatible with Quarkus native if in the future we want to use it).
The drawback is that it lacks many features that regular expression parsers have like conditional expressions and many others.
Another alternative to SPeL expressions and Quarkus Qute is to use Mvel2 (which is supported by JBoss RedHat team). This framework works in Spring Boot and Quarkus, but not in Quarkus native, see the Quarkus example I created for this - use
mvn clean installfrom the mvel2 directory andmvn clean install -Dnativeto run the test using the native binaries and you can see the test will fail).Moreover, because of using Mvel2 or Qute (or any other expression parsers), we would need to update all the properties to accommodate the new conventions (for example, instead of using
#{metric.id}, we need to use@{metric.id}in Mvel2.Therefore, using this very simple solution, it works in Quarkus native (because it does not use reflection), also it does not require changes and it's very well performant (because of the same, neither uses reflection nor requires adding an additional dependency).