Conversation
carlosantoniodasilva
left a comment
There was a problem hiding this comment.
Nice cleanup, just a few bits of food for thought.
| String configuredUrl = getApiBaseUrl(); | ||
| return configuredUrl != null && !configuredUrl.trim().isEmpty(); | ||
| } | ||
| public class JudoscaleConfig extends ConfigBase { |
There was a problem hiding this comment.
Do these still need to have their own class rather than using the config one? Seems like it's still necessary due to specific framework things?
There was a problem hiding this comment.
yeah, it's the @ConfigurationProperties line above that's specific to Spring Boot.
@ConfigurationProperties is Spring Boot's way of automatically binding external configuration to Java objects. With @ConfigurationProperties(prefix = "judoscale"):
# application.properties
judoscale.api-base-url=https://api.judoscale.com/abc123
or environment variables:
JUDOSCALE_URL=https://api.judoscale.com/abc123
...automatically get mapped to the corresponding setters (setApiBaseUrl(), etc.).
There was a problem hiding this comment.
Gotcha, thanks for clarifying 👍
| ObjectNode adapters = objectMapper.createObjectNode(); | ||
| ObjectNode springBootAdapter = objectMapper.createObjectNode(); | ||
| springBootAdapter.put("adapter_version", adapterVersion); | ||
| adapters.set("judoscale-spring-boot", springBootAdapter); |
There was a problem hiding this comment.
Should this handle multiple adapters somehow? I think our report builder on other packages pass the list of adapters if there are multiple (it's not the case on the java package yet because we can only have one, but it might be in the future)
There was a problem hiding this comment.
Yes definitely and this was an oversight. Core should not have any knowledge about the SpringBootAdapter. Good catch!
| private static final ObjectMapper objectMapper = new ObjectMapper(); | ||
| private static final int MAX_RETRIES = 3; | ||
| private static final String ADAPTER_VERSION = loadAdapterVersion(); | ||
| private static final String ADAPTER_VERSION = ReportBuilder.loadAdapterVersion(JudoscaleApiClient.class); |
There was a problem hiding this comment.
We might be able to extract more Api client functionality into core to send these requests, but I think we might need to handle the adapters reporting in the process as well (see my other comment).
There was a problem hiding this comment.
The challenge with the API client is that our core library needs to support Java 8 as does the Spring Boot 2 starter, but the Spring Boot starter can use the built-in HTTP client from Java 11. For now I've kept them separate.
There was a problem hiding this comment.
I see, makes sense... well I guess in the future we could revisit so that each lib provides the http interface, and core provides the base api client that handles the flow of request & retrying.... anyway, problem for later.
There was a problem hiding this comment.
FWIW, this is me playing with Claude 😄: a refactoring extracting a base class, where each implementation handles the http stuff: https://github.com/judoscale/judoscale-java/compare/ca-refactor-api-client
| String configuredUrl = getApiBaseUrl(); | ||
| return configuredUrl != null && !configuredUrl.trim().isEmpty(); | ||
| } | ||
| public class JudoscaleConfig extends ConfigBase { |
There was a problem hiding this comment.
yeah, it's the @ConfigurationProperties line above that's specific to Spring Boot.
@ConfigurationProperties is Spring Boot's way of automatically binding external configuration to Java objects. With @ConfigurationProperties(prefix = "judoscale"):
# application.properties
judoscale.api-base-url=https://api.judoscale.com/abc123
or environment variables:
JUDOSCALE_URL=https://api.judoscale.com/abc123
...automatically get mapped to the corresponding setters (setApiBaseUrl(), etc.).
| ObjectNode adapters = objectMapper.createObjectNode(); | ||
| ObjectNode springBootAdapter = objectMapper.createObjectNode(); | ||
| springBootAdapter.put("adapter_version", adapterVersion); | ||
| adapters.set("judoscale-spring-boot", springBootAdapter); |
There was a problem hiding this comment.
Yes definitely and this was an oversight. Core should not have any knowledge about the SpringBootAdapter. Good catch!
| private static final ObjectMapper objectMapper = new ObjectMapper(); | ||
| private static final int MAX_RETRIES = 3; | ||
| private static final String ADAPTER_VERSION = loadAdapterVersion(); | ||
| private static final String ADAPTER_VERSION = ReportBuilder.loadAdapterVersion(JudoscaleApiClient.class); |
There was a problem hiding this comment.
The challenge with the API client is that our core library needs to support Java 8 as does the Spring Boot 2 starter, but the Spring Boot starter can use the built-in HTTP client from Java 11. For now I've kept them separate.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
e02864c to
6df6964
Compare
We ended up with a lot of duplicated code when we added judoscale-spring-boot-2-starter. This PR extracts what we can into judoscale-core.
There's still some functionality that feels like it should be in core (like the API clients), but we're constrained that core must remain compatible with Java 8, which means we can't use
java.net.http.HttpClientlike judoscale-spring-boot-starter can.Note
Medium Risk
Refactors core metrics reporting/serialization and request queue-time parsing used by both starters, so regressions could impact telemetry accuracy or reporting behavior despite largely equivalent logic.
Overview
Extracts duplicated functionality into
judoscale-coreby addingConfigBase,QueueTimeCalculator,ReportBuilder,Reporter, and anAdaptermodel (with Jackson added as a core dependency).Updates both
judoscale-spring-boot-starterandjudoscale-spring-boot-2-starterto extend/use these core utilities: configs now inherit fromConfigBase, filters callQueueTimeCalculator(and warn on unparseable headers), API clients build payloads viaReportBuilder(including per-starter adapter name/version), and Spring reporters become thin wrappers over coreReporter. Adds comprehensive unit tests in core and removes now-redundant JSON-building tests from the Spring Boot starter.Written by Cursor Bugbot for commit 6df6964. This will update automatically on new commits. Configure here.