Commit declarative config schema pojos to git#8408
Conversation
There was a problem hiding this comment.
These are the changes you really want to review. The shape of the pojos themselves are the result of the jsonschema2pojo config defined in this file.
| || ((this.exporter != null) && this.exporter.equals(rhs.exporter)))) | ||
| && ((this.exportTimeout == rhs.exportTimeout) | ||
| || ((this.exportTimeout != null) | ||
| && this.exportTimeout.equals(rhs.exportTimeout)))) |
There was a problem hiding this comment.
This is an example (line 207) of the boxed primitive comparison which is flagged by error prone but which we suppress. Its not a real problem because it later does an equals check.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (40.77%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8408 +/- ##
============================================
- Coverage 91.04% 82.14% -8.90%
- Complexity 7822 8607 +785
============================================
Files 893 1008 +115
Lines 23721 28824 +5103
Branches 2364 3569 +1205
============================================
+ Hits 21597 23678 +2081
- Misses 1406 4148 +2742
- Partials 718 998 +280 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| }) | ||
| @Generated("jsonschema2pojo") | ||
| @SuppressWarnings({"NullAway", "rawtypes", "BoxedPrimitiveEquality"}) | ||
| public class BatchSpanProcessorModel { |
There was a problem hiding this comment.
No constructor means jackson is currently relying on initializing using the public constructor, and using a reflection approval to identify all the fields and set using @JsonProperty annotations.
I want to see if we can / should change this.
| return maxQueueSize; | ||
| } | ||
|
|
||
| public BatchSpanProcessorModel withMaxQueueSize(Integer maxQueueSize) { |
There was a problem hiding this comment.
These with* methods are convenient, but are more like setters because they don't create a new immutable instance each time their called. Should investigate whether we can configure to name the set* instead.
| StringBuilder sb = new StringBuilder(); | ||
| sb.append(BatchSpanProcessorModel.class.getName()) | ||
| .append('@') | ||
| .append(Integer.toHexString(System.identityHashCode(this))) |
There was a problem hiding this comment.
I don't like this toString implementation which includes the identity hash. Should look into whether its configurable to disable. Also, the "" and bracket formatting (i.e. "[=]") are inconsistent with other toString implementations in this repo, so should look into that as well.
jaydeluca
left a comment
jaydeluca
left a comment
There was a problem hiding this comment.
probably doesn't matter too much, but when I ran this locally I got a diff on a dozen or so files because of hashcode and equals ordering. I was on Java 17 and when I switched to 21 and regenerated it, it fixed it
…y-java into commit-declarative-config-pojos
|
Thanks @jaydeluca I pushed a commit which adds a check to |
|
Hey @open-telemetry/java-approvers - will merge this at end of week if nobody has any concerns. |
2 participants
Related to #8402
Currently, we publish config schema pojos in our artifacts but they are produced to a generated source set which is not checked into version control.
As we look to stabilize, we need to be able to see these pojos so that"
This commits generated pojos to version control as is. Or rather, with as few changes as possible:
src/main/javainstead of generated source set.equals(..). TODO to look into if alternative equals implementations are possible.In a followup PR, I'm going to rework the generation to produce experimental types to an internal package.