Adding OpenTelemetry observability features #326
Conversation
Signed-off-by: Maximilian Inckmann <[email protected]>
Signed-off-by: Maximilian Inckmann <[email protected]>
Signed-off-by: Maximilian Inckmann <[email protected]>
Signed-off-by: Maximilian Inckmann <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces observability via OpenTelemetry/Micrometer across the codebase, updates Spring Boot to 3.5.4, adds OTLP collector config, migrates logging to Log4j2 with an OTEL appender, and refactors batch PID creation API/flow. Several classes gain @Observed/@WithSpan, plus timing/counting metrics. Some behavior changes occur in batch creation and local prefix filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as TypingRESTResourceImpl
participant S as TypingService
participant IDS as IdentifierSystem
participant TR as TypeRegistry
participant E as ElasticRepo
participant MQ as Messaging
participant OTEL as OTLP Collector
C->>R: POST /pids (batch)
activate R
R->>R: generatePIDMapping()
R->>R: applyMappingsAndValidate()
alt dry-run
R-->>C: 200 OK + List<PIDRecord>
else register
R->>IDS: registerPidUnchecked()
IDS-->>R: PID
R->>S: enrich + validate
S->>TR: lookups
TR-->>S: metadata
S-->>R: enriched record
R->>E: save()
R->>MQ: publish event
R-->>C: 201 Created + List<PIDRecord>
end
R-->>OTEL: traces/metrics/logs
deactivate R
sequenceDiagram
participant APP as Application
participant OTEL as OpenTelemetry SDK
participant COL as OTLP Collector
APP->>OTEL: Spans (WithSpan/@Observed)
APP->>OTEL: Metrics (@Timed/@Counted)
APP->>OTEL: Logs (Log4j2 OTEL Appender)
OTEL-->>COL: OTLP HTTP 4318 (traces/metrics/logs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (9)
config/application-default.properties (1)
90-90: Hardcoded secrets in default config (JWT secret and DB credentials)Committing credentials invites accidental misuse in non-dev environments and leaks in telemetry/logs.
Replace with environment placeholders and avoid shipping real-looking secrets:
-repo.auth.jwtSecret=vkfvoswsohwrxgjaxipuiyyjgubggzdaqrcuupbugxtnalhiegkppdgjgwxsmvdb +## Use environment variable in all environments; don't commit real secrets. +repo.auth.jwtSecret=${REPO_AUTH_JWT_SECRET:} @@ -spring.datasource.username=typid -spring.datasource.password=secure_me +## Externalize DB credentials +spring.datasource.username=${DB_USERNAME:} +spring.datasource.password=${DB_PASSWORD:}Optionally, document local dev defaults in a sample file (application-local.properties.example) rather than in the default profile.
Also applies to: 286-287
src/main/java/edu/kit/datamanager/pit/pitservice/impl/EmbeddedStrictValidatorStrategy.java (1)
141-146: Preserve original cause when wrapping as ExternalServiceExceptionCurrently the cause is dropped, making issues harder to troubleshoot.
- throw new ExternalServiceException(this.typeRegistry.getRegistryIdentifier()); + throw new ExternalServiceException(this.typeRegistry.getRegistryIdentifier(), e);src/main/java/edu/kit/datamanager/pit/pidsystem/impl/local/LocalPidSystem.java (1)
118-125: Avoid tracing full records: remove @SpanAttribute on PIDRecordAnnotating PIDRecord risks leaking full record content in spans.
- public String registerPidUnchecked(@SpanAttribute final PIDRecord pidRecord) throws PidAlreadyExistsException, ExternalServiceException { + public String registerPidUnchecked(final PIDRecord pidRecord) throws PidAlreadyExistsException, ExternalServiceException { @@ - public boolean updatePid(@SpanAttribute PIDRecord rec) throws PidNotFoundException, ExternalServiceException, RecordValidationException { + public boolean updatePid(PIDRecord rec) throws PidNotFoundException, ExternalServiceException, RecordValidationException {Set only a pid attribute within the span if needed.
Also applies to: 131-137
src/main/java/edu/kit/datamanager/pit/pidsystem/impl/handle/HandleProtocolAdapter.java (3)
181-186: Guard against null adminValue and non-admin mode in registerPidUncheckedWhen no credentials are configured, adminValue remains null. Adding it to the admin list (Line 185) risks a NullPointerException or unintended behavior. Also, write operations should be rejected in non-admin mode.
Apply this diff to reject write operations without admin credentials and avoid nulls:
public String registerPidUnchecked(@SpanAttribute final PIDRecord pidRecord) throws PidAlreadyExistsException, ExternalServiceException { - // Add admin value for configured user only - // TODO add options to add additional adminValues e.g. for user lists? - ArrayList<HandleValue> admin = new ArrayList<>(); - admin.add(this.adminValue); + if (!this.isAdminMode) { + throw new ExternalServiceException(SERVICE_NAME_HANDLE, "Adapter is not configured with administrative credentials."); + } + // Add admin value for configured user only + // TODO add options to add additional adminValues e.g. for user lists? + ArrayList<HandleValue> admin = new ArrayList<>(); + if (this.adminValue != null) { + admin.add(this.adminValue); + }
151-158: queryPid should not return null on empty values; throw PidNotFoundExceptionReturning null violates the usual contract (other implementations throw PidNotFoundException). This can propagate nulls and cause NPEs.
Use a not-found exception instead:
- if (allValues.isEmpty()) { - return null; - } + if (allValues.isEmpty()) { + throw new PidNotFoundException(pid); + }
165-168: Null safety for resolveHandle returnHandleResolver/HSAdapter may return null arrays; Stream.of(values) would NPE.
Protect against null/empty:
- HandleValue[] values = this.client.resolveHandle(pid, null, null); - return Stream.of(values) - .collect(Collectors.toCollection(ArrayList::new)); + HandleValue[] values = this.client.resolveHandle(pid, null, null); + if (values == null || values.length == 0) { + throw new PidNotFoundException(pid); + } + return Stream.of(values).collect(Collectors.toCollection(ArrayList::new));src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java (1)
94-99: Null safety for resolveHandle resultArrays.stream(this.client.resolveHandle(...)) will NPE if the adapter returns null for unknown PIDs. In HandleProtocolAdapter.queryAllHandleValues you already guard for null/empty.
Handle null/empty before streaming:
- Collection<HandleValue> recordProperties = Arrays.stream(this.client.resolveHandle(pid, null, null)) + HandleValue[] arr = this.client.resolveHandle(pid, null, null); + if (arr == null || arr.length == 0) { + throw new PidNotFoundException(pid); + } + Collection<HandleValue> recordProperties = Arrays.stream(arr) .filter(value -> !HandleBehavior.isHandleInternalValue(value)) .collect(Collectors.toList());src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (1)
84-88: Fix URI building: insert missing slash betweenschemaand PID (prevents 404s)Confirmed: UriComponentsBuilder.pathSegment(...) + .path(value) does not insert a separator. With maybeTypePid =
21.T11998/abcthe current code yields/v1/types/schema21.T11998/abc(missing slash). Also note pathSegment(...) would percent‑encode slashes if used for the PID (not desired if PID includes path separators).Files to fix:
- src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java — around lines 85–86
Suggested change:
- .uri(uriBuilder -> uriBuilder - .pathSegment("schema") - .path(maybeTypePid) - .build()) + .uri(uriBuilder -> uriBuilder + .pathSegment("schema") + .path("/") + .path(maybeTypePid) + .build())After this change, a PID
21.T11998/abcwill produce the expected path/v1/types/schema/21.T11998/abc.src/main/java/edu/kit/datamanager/pit/typeregistry/impl/TypeApi.java (1)
169-177: Fix compile error and resource leak in queryProfile: wrap InputStream in try-with-resources and handle IOException.InputStream.readAllBytes() and close() throw IOException, but they are not caught or declared here, causing a compilation error and a potential leak on error paths.
Apply this diff:
- if (clientResponse.getStatusCode().is2xxSuccessful()) { - InputStream inputStream = clientResponse.getBody(); - String body = new String(inputStream.readAllBytes()); - inputStream.close(); - return extractProfileInformation(maybeProfilePid, Application.jsonObjectMapper().readTree(body)); - } else { + if (clientResponse.getStatusCode().is2xxSuccessful()) { + try (InputStream inputStream = clientResponse.getBody()) { + return extractProfileInformation( + maybeProfilePid, + Application.jsonObjectMapper().readTree(inputStream)); + } catch (IOException e) { + throw new TypeNotFoundException(maybeProfilePid); + } + } else { throw new TypeNotFoundException(maybeProfilePid); }
🧹 Nitpick comments (47)
src/main/java/edu/kit/datamanager/pit/pidsystem/IIdentifierSystem.java (8)
36-36: Fix typo in JavaDoc comment.There's a typo in the documentation: "exaxtly" should be "exactly".
- * with a the prefix. The returnes String is then exaxtly the same. + * with the prefix. The returned String is then exactly the same.
40-40: Fix incomplete JavaDoc comment.The JavaDoc comment contains an incomplete sentence.
- * @throws InvalidConfigException if the system can n. + * @throws InvalidConfigException if the system cannot append a prefix.
56-56: Fix typo in JavaDoc comment.There's a typo: "commonication" should be "communication".
- * @throws ExternalServiceException on commonication errors or errors on other + * @throws ExternalServiceException on communication errors or errors on other
70-70: Fix typo in JavaDoc comment.Same typo as above: "commonication" should be "communication".
- * @throws ExternalServiceException on commonication errors or errors on other + * @throws ExternalServiceException on communication errors or errors on other
88-88: Fix typo in JavaDoc comment.Another instance of the same typo.
- * @throws ExternalServiceException on commonication errors or errors on other + * @throws ExternalServiceException on communication errors or errors on other
103-103: Fix typo in JavaDoc comment.Typo: "occured" should be "occurred".
- * @throws ExternalServiceException if an error occured in communication with + * @throws ExternalServiceException if an error occurred in communication with
131-131: Fix typo in JavaDoc comment.Same typo as above.
- * @throws ExternalServiceException if an error occured in communication with + * @throws ExternalServiceException if an error occurred in communication with
142-142: Fix typo in JavaDoc comment.Another instance of the same typo.
- * @throws ExternalServiceException if an error occured in communication with + * @throws ExternalServiceException if an error occurred in communication withsrc/main/resources/log4j2.xml (1)
1-11: Consider adding a console appender for local development.Having only the OpenTelemetry appender may make local development difficult when the OTLP collector is not running.
Add a console appender for fallback or development scenarios:
<Configuration status="WARN" packages="io.opentelemetry.instrumentation.log4j.appender.v2_17"> <Appenders> <OpenTelemetry name="OpenTelemetryAppender"/> + <Console name="Console" target="SYSTEM_OUT"> + <PatternLayout pattern="%d{HH:mm:ss.SSS} [%t] %-5level %logger{36} - %msg%n"/> + </Console> </Appenders> <Loggers> <Root> <AppenderRef ref="OpenTelemetryAppender" level="All"/> + <!-- Uncomment for local development --> + <!-- <AppenderRef ref="Console" level="INFO"/> --> </Root> </Loggers> </Configuration>build.gradle (1)
207-215: Inconsistent formatting in JaCoCo exclusion list.The exclusion list has inconsistent backslash placement and alignment, making it harder to read and maintain.
- fileTree(dir: it, exclude: [\ - 'edu/kit/datamanager/pit/configuration/**', \ - 'edu/kit/datamanager/pit/web/converter/**', \ - 'edu/kit/datamanager/pit/web/ExtendedErrorAttributes**', \ - 'edu/kit/datamanager/pit/web/UncontrolledExceptionHandler**', \ - 'edu/kit/datamanager/pit/common/**', \ - 'edu/kit/datamanager/pit/Application*' + fileTree(dir: it, exclude: [ + 'edu/kit/datamanager/pit/configuration/**', + 'edu/kit/datamanager/pit/web/converter/**', + 'edu/kit/datamanager/pit/web/ExtendedErrorAttributes**', + 'edu/kit/datamanager/pit/web/UncontrolledExceptionHandler**', + 'edu/kit/datamanager/pit/common/**', + 'edu/kit/datamanager/pit/Application*' ])src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java (4)
69-71: Consider making the media type check more robust.The current implementation uses
contains()which could match unintended media types. For example, a media type like "application/vnd.datamanager.pid.simple.extended+json" would incorrectly match.private boolean isValidMediaType(MediaType arg1) { - return arg1.toString().contains(SimplePidRecord.CONTENT_TYPE_PURE); + return arg1.getType().equals("application") && + arg1.getSubtype().equals(SimplePidRecord.CONTENT_TYPE_PURE + "+json"); }
74-80: Null check on MediaType is unnecessary.The null check for
arg1is redundant sinceisValidMediaType()would handle null by throwing a NullPointerException. Spring framework typically doesn't pass null MediaType to these methods.@Override public boolean canRead(Class<?> arg0, MediaType arg1) { - if (arg0 == null || arg1 == null) { + if (arg0 == null) { return false; } LOGGER.trace("canRead: Checking applicability for class {} and mediatype {}.", arg0, arg1); return PIDRecord.class.equals(arg0) && isValidMediaType(arg1); }
95-104: Consider adding error handling for malformed JSON.The
readmethod could throw a more descriptive exception when JSON parsing fails, helping with debugging and monitoring.@Override @Counted(value = "simplePidRecordConverter.read.count", description = "Number of reads of SimplePidRecord") public PIDRecord read(Class<? extends PIDRecord> arg0, HttpInputMessage arg1) throws IOException, HttpMessageNotReadableException { LOGGER.trace("Read simple message from client and convert to PIDRecord."); try (InputStreamReader reader = new InputStreamReader(arg1.getBody(), StandardCharsets.UTF_8)) { String data = new BufferedReader(reader).lines().collect(Collectors.joining("\n")); - return new PIDRecord(Application.jsonObjectMapper().readValue(data, SimplePidRecord.class)); + try { + SimplePidRecord simplePidRecord = Application.jsonObjectMapper().readValue(data, SimplePidRecord.class); + return new PIDRecord(simplePidRecord); + } catch (IOException e) { + throw new HttpMessageNotReadableException("Failed to parse SimplePidRecord JSON: " + e.getMessage(), e, arg1); + } } }
106-114: Consider setting content length header for better HTTP compliance.Setting the Content-Length header improves HTTP compliance and helps clients handle the response more efficiently.
@Override @Counted(value = "simplePidRecordConverter.write.count", description = "Number of writes of SimplePidRecord") public void write(PIDRecord arg0, MediaType arg1, HttpOutputMessage arg2) throws IOException, HttpMessageNotWritableException { LOGGER.trace("Write PIDRecord to simple format for client."); SimplePidRecord sim = new SimplePidRecord(arg0); byte[] simSerialized = Application.jsonObjectMapper().writeValueAsBytes(sim); + arg2.getHeaders().setContentLength(simSerialized.length); arg2.getBody().write(simSerialized); }src/main/java/edu/kit/datamanager/pit/pitservice/impl/TypingService.java (3)
151-155: Exception mapping in enrichment loses root cause and differs from validation strategyThe catch block wraps any Completion/Cancellation into a generic InvalidConfigException, masking the cause and diverging from the more robust unwrapping in EmbeddedStrictValidatorStrategy.
Consider aligning exception handling and preserving the root cause:
- } catch (CompletionException | CancellationException ex) { - // TODO convert exceptions like in validation service. - throw new InvalidConfigException(LOG_MSG_TYPING_SERVICE_MISCONFIGURED); - } + } catch (CompletionException | CancellationException ex) { + // Align behavior with validation strategy and preserve cause + LOG.warn("Failed to enrich attribute names from type registry. {}", LOG_MSG_TYPING_SERVICE_MISCONFIGURED, ex); + throw new InvalidConfigException(LOG_MSG_TYPING_SERVICE_MISCONFIGURED, ex); + }If you want identical behavior to validation, extract the unwrapping logic into a shared utility and reuse it here.
87-89: Reassess the need for both @timed and @counted; Timer already provides a countMicrometer Timer publishes a count metric (e.g., _seconds_count). Having separate @counted for the same operation can duplicate metrics.
Drop @counted where a Timer is present unless you want a distinct counter with different tags or semantics.
Also applies to: 95-99, 103-107, 113-117, 121-127, 129-133, 165-171, 173-179, 181-186
54-55: Unused constant LOG_MSG_QUERY_TYPEThis constant appears unused.
Remove it if not intended for a follow-up to reduce noise.
- private static final String LOG_MSG_QUERY_TYPE = "Querying for type with identifier {}.";config/application-default.properties (3)
349-351: Conflicting/split sampling configuration (management.tracing and otel.traces.sampler)You set both management.tracing.sampling.probability and otel.traces.sampler(+arg). This can lead to confusion about the effective sampler.
Pick one configuration source for clarity. Recommended options:
- Keep otel.traces.sampler/arg and remove management.tracing.sampling.probability, or
- Use Boot management.tracing.* exclusively and drop otel.traces.sampler/arg.
Example (keep OTEL sampler):
-management.tracing.sampling.probability=0.1 -# management.tracing.opentelemetry.export.include-unsampled=true @@ -# otel.traces.sampler.arg=1 +otel.traces.sampler=parentbased_traceidratio +otel.traces.sampler.arg=0.1Also applies to: 367-369
299-299: Prometheus access is unrestricted while Prometheus export is disabledExposing the endpoint when exporter is disabled is inconsistent.
Either disable the endpoint access config or enable the exporter. For example:
-management.endpoint.prometheus.access=unrestricted -management.prometheus.metrics.export.enabled=false +# management.endpoint.prometheus.access=unrestricted +management.prometheus.metrics.export.enabled=falseAlso applies to: 335-336
331-334: Potential null/empty environment tagmanagement.metrics.tags.environment=${spring.profiles.active} may resolve to empty when no profile is active, creating ambiguous series.
Provide a default:
-management.metrics.tags.environment=${spring.profiles.active} +management.metrics.tags.environment=${spring.profiles.active:default}src/main/java/edu/kit/datamanager/pit/pitservice/impl/EmbeddedStrictValidatorStrategy.java (3)
68-76: Javadoc is outdated: missing pidRecord parameterThe method signature now includes pidRecord but Javadoc lists only e.
- * @param e the exception to unwrap. + * @param pidRecord the record being validated (for context in error mapping). + * @param e the exception to unwrap.
77-90: Unwrapping loop skips checking the initial exception instanceThe loop advances to cause before checking types, missing cases where e itself is a RecordValidationException or TypeNotFoundException.
- Throwable cause = e; - - for (int level = 0; level <= MAX_LEVEL && cause != null; level++) { - cause = cause.getCause(); + Throwable cause = e; + for (int level = 0; level <= MAX_LEVEL && cause != null; level++) { if (cause instanceof RecordValidationException rve) { throw rve; } else if (cause instanceof TypeNotFoundException tnf) { throw new RecordValidationException( pidRecord, "Type not found: %s".formatted(tnf.getMessage())); } + cause = cause.getCause(); }
93-96: Consider dropping @counted where @timed is presentAs with other classes, Timer already emits a count metric.
Remove redundant @counted unless you need a distinct counter with different semantics.
src/main/java/edu/kit/datamanager/pit/pidsystem/impl/local/LocalPidSystem.java (2)
98-101: SpanKind choice: prefer INTERNAL for local DB operationsThese operations interact with an in-process DAO; marking them CLIENT may misrepresent trace topology.
Switch to SpanKind.INTERNAL for internal boundaries. Example:
- @WithSpan(kind = SpanKind.CLIENT) + @WithSpan(kind = SpanKind.INTERNAL)Also applies to: 106-111, 115-118, 128-131, 140-145, 148-151
152-156: parallelStream here is unnecessary and may add overheadConverting a DB-backed collection to parallel stream rarely helps for simple map/filter and set collection, and can increase contention.
- return this.db.findAll().parallelStream() + return this.db.findAll().stream() .map(dbo -> dbo.getPid()) .filter(pid -> pid.startsWith(PREFIX)) .collect(Collectors.toSet());src/main/java/edu/kit/datamanager/pit/pidsystem/impl/handle/HandleProtocolAdapter.java (2)
181-182: Avoid using @SpanAttribute on complex objects (PIDRecord)OpenTelemetry attributes should be scalar types (string, number, boolean, arrays thereof). Annotating complex objects (PIDRecord) will serialize toString, producing high-cardinality, low-value attributes.
- Remove @SpanAttribute from PIDRecord parameters.
- Instead, add selected attributes inside the method:
Span.current().setAttribute("pid", pidRecord.getPid());
This reduces cardinality and improves trace usefulness.Also applies to: 211-213
355-362: Typo in Javadoc“expeted” -> “expected”.
- * - If a generator prefix is set, the PID is expeted to have this prefix. + * - If a generator prefix is set, the PID is expected to have this prefix.src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java (2)
100-107: Variable name misleading (isExistingPid is false on not-found)
isExistingPidis true when the code equals HANDLE_DOES_NOT_EXIST. Consider renaming todoesNotExistor similar to improve readability.No functional change required.
81-87: Simplify prefix extraction for readabilityThe
Arrays.stream(pid.split("/", 2)).findFirst()indirection is unnecessary.Readable alternative:
int idx = pid.indexOf('/');- if idx < 0 -> throw PidNotFoundException
- else prefix = pid.substring(0, idx + 1);
src/main/java/edu/kit/datamanager/pit/pidsystem/impl/InMemoryIdentifierSystem.java (2)
90-91: Avoid @SpanAttribute on PIDRecord parametersAnnotating complex objects increases attribute cardinality and may serialize entire records to attribute values.
- Remove @SpanAttribute from PIDRecord parameters.
- If useful, set targeted attributes inside the method:
Span.current().setAttribute("pid", record.getPid());Also applies to: 100-101
90-94: Missing duplicate check in registerPidUncheckedInterface suggests throwing PidAlreadyExistsException on duplicates. This in-memory impl silently overwrites.
Consider:
- this.records.put(pidRecord.getPid(), pidRecord); + if (this.records.containsKey(pidRecord.getPid())) { + throw new PidAlreadyExistsException(pidRecord.getPid()); + } + this.records.put(pidRecord.getPid(), pidRecord);If intentional for tests, ignore.
src/main/java/edu/kit/datamanager/pit/web/impl/TypingRESTResourceImpl.java (1)
392-393: Fix typos in comments: deprecated/prependMinor doc fixes.
- "", // TODO parameter is depricated and will be removed soon. + "", // TODO parameter is deprecated and will be removed soon.- // The registration at the PID service will preprend the prefix. + // The registration at the PID service will prepend the prefix.Also applies to: 331-333
src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (1)
79-83: Avoid duplicating spans/metrics: prefer one instrumentation styleYou’re combining
@Observed(class-level) with@WithSpan,@Timed, and@Countedon the method. This often creates nested/duplicate spans and redundant metrics. Given Spring Boot Micrometer -> OpenTelemetry bridge,@Observed+@Timedalready produce tracing and timing.Recommendation:
- Keep
@Observed+@Timed, and drop@WithSpanand possibly@Counted(Timer already exposes count).- Or, if you prefer OpenTelemetry annotations only, remove
@Observedand@Timedand keep@WithSpan.If you keep both
@Timedand@Counted, you’ll count the same event twice via different meters. Consider using only@Timed.- @WithSpan(kind = SpanKind.CLIENT) - @Timed(value = "typeapi_schema_generator_generate", description = "Time taken to generate schema from Type API") - @Counted(value = "typeapi_schema_generator_generate_total", description = "Total number of Type API schema generations") + @Timed(value = "typeapi_schema_generator_generate", description = "Time taken to generate schema from Type API")Also consider standardizing metric names to dot notation (e.g.,
pit.schema.typeapi.generate) for consistency.src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenPrefixed.java (1)
41-45: Uppercasing here changes behavior; verify intent or drop to keep single-responsibility
PidSuffixGenPrefixednow forces upper-case, which overlaps withPidSuffixGenUpperCaseand likely violates the “prefixed only” responsibility. If uppercasing wasn’t intended here, remove it and let callers compose withPidSuffixGenUpperCasewhen needed.Also, consider making prefixing idempotent to avoid accidental double-prefixing if upstream generators already add it.
- public PidSuffix generate() { - String suffix = this.generator.generate().get().toUpperCase(); - return new PidSuffix(this.prefix.concat(suffix)); - } + public PidSuffix generate() { + String raw = this.generator.generate().get(); + String suffixed = PidSuffix.asPrefixedChecked(raw, this.prefix); + return new PidSuffix(suffixed); + }Would you prefer keeping case normalization here? If yes, consider renaming to
PidSuffixGenPrefixedUpperCasefor clarity.src/main/java/edu/kit/datamanager/pit/typeregistry/schema/DtrTestSchemaGenerator.java (2)
100-101: Nit: missing space in error messageThe concatenation yields
dtr-testcould not create .... Add a space afterORIGIN.- throw new IOException(ORIGIN + "could not create valid schema for %s from %s " + throw new IOException(ORIGIN + " could not create valid schema for %s from %s "
83-86: Avoid redundant spans/metrics as in other generatorsSame note as TypeApiSchemaGenerator: combining
@Observed,@WithSpan,@Timed, and@Countedtypically duplicates spans/metrics. Prefer@Observed+@Timed, and drop@WithSpan/@Countedunless you explicitly need separate counters.- @WithSpan(kind = SpanKind.CLIENT) - @Timed(value = "dtr_test_schema_generate", description = "Time taken to generate schema from DTR Test") - @Counted(value = "dtr_test_schema_generate_total", description = "Total number of DTR Test schema generations") + @Timed(value = "dtr_test_schema_generate", description = "Time taken to generate schema from DTR Test")src/main/java/edu/kit/datamanager/pit/pidgeneration/PidSuffix.java (2)
23-27: Typo: “accidentially” → “accidentally”Minor Javadoc fix.
- * not used as a PID accidentially. + * not used as a PID accidentally.
45-52: Over-instrumentation: remove spans from trivial accessors/helpersAnnotating tiny methods (
get(),getWithPrefix, andasPrefixedChecked) will create very high-volume spans with little diagnostic value and non-trivial overhead. Keep spans coarse (call sites or higher-level operations).- @WithSpan(kind = SpanKind.INTERNAL) public static String asPrefixedChecked(String maybeSuffix, String prefix) { if (!maybeSuffix.startsWith(prefix)) { return prefix + maybeSuffix; } else { return maybeSuffix; } } @@ - @WithSpan(kind = SpanKind.INTERNAL) public String get() { return suffix; } @@ - @WithSpan(kind = SpanKind.INTERNAL) public String getWithPrefix(String prefix) { return prefix + suffix; }If you want metrics around prefixing calls, consider timing/counters at the generator layer instead.
Also applies to: 59-62, 70-73
src/main/java/edu/kit/datamanager/pit/typeregistry/schema/SchemaSetGenerator.java (2)
81-86: @timed on an async method is misleading (records near-zero durations). Instrument the loader or wrap the future instead.generateFor returns a CompletableFuture immediately, so @timed will record only the synchronous time to submit, not the actual schema generation. Either time the cache loader body or wrap the returned future and stop the timer on completion.
Apply this minimal change if you prefer to avoid misleading timings until async timing is added:
@WithSpan(kind = SpanKind.INTERNAL) -@Timed(value = "schema_generator_generate_for", description = "Time taken to generate schemas for attribute") @Counted(value = "schema_generator_generate_for_total", description = "Total number of schema generation requests") public CompletableFuture<Set<SchemaInfo>> generateFor(@SpanAttribute final String attributePid) { return this.CACHE.get(attributePid); }If you want accurate async timing, one option is to inject MeterRegistry and wrap the future:
// Outside selected lines: example only @Autowired MeterRegistry registry; public CompletableFuture<Set<SchemaInfo>> generateFor(@SpanAttribute final String attributePid) { Timer.Sample sample = Timer.start(registry); return this.CACHE.get(attributePid) .whenComplete((res, ex) -> sample.stop(Timer.builder("schema_generator_generate_for") .description("Time taken to generate schemas for attribute") .tag("success", Boolean.toString(ex == null)) .register(registry))); }
39-41: Avoid double-instrumentation: @observed + method-level @WithSpan/@Timed/@counted can create duplicate spans/metrics.Class-level @observed will wrap all public methods with an Observation (and possibly bridge to traces), while you also annotate methods with @WithSpan/@Timed/@counted. This often results in nested or duplicated spans and metrics.
If you intend to use explicit method instrumentation, consider removing @observed here:
-@Observed public class SchemaSetGenerator {Alternatively, drop the explicit @WithSpan/@Timed/@counted and rely on @observed conventions. Please verify with your OTEL/Micrometer backend that you’re not seeing double spans or duplicate meters before merging.
src/main/java/edu/kit/datamanager/pit/typeregistry/impl/TypeApi.java (7)
114-116: Fix log message for attribute cache removal (wrong entity names).This is the attributeCache; the log still says "profile ... profile cache".
- LOG.trace("Removing profile {} from profile cache. Cause: {}", key, cause) + LOG.trace("Removing attribute {} from attribute cache. Cause: {}", key, cause)
133-136: Avoid unnecessary String allocation; parse JSON directly from the InputStream.Saves memory and reduces GC pressure.
- try (InputStream inputStream = clientResponse.getBody()) { - String body = new String(inputStream.readAllBytes()); - return extractAttributeInformation(attributePid, Application.jsonObjectMapper().readTree(body)); - } catch (IOException e) { + try (InputStream inputStream = clientResponse.getBody()) { + return extractAttributeInformation( + attributePid, + Application.jsonObjectMapper().readTree(inputStream)); + } catch (IOException e) {
58-60: Potential duplicate spans/metrics: @observed on class plus @WithSpan/@Timed/@counted on methods.Like in SchemaSetGenerator, this can create nested spans and duplicate meters. Choose one approach for consistency and to reduce noise.
Consider removing @observed if you keep explicit annotations:
-@Observed public class TypeApi implements ITypeRegistry {If you prefer @observed conventions throughout, drop the method-level tracing/metrics and configure a global ObservationConvention.
81-91: “Long http request” timing measures only request execution until headers, not body read.The interceptor measures time for execution.execute(request, body). The actual payload reading is done later in the .exchange() lambdas, so very large bodies won’t be reflected here. If you intend to measure end-to-end latency, consider moving the measurement around the body parsing or rely on @timed around the method.
Would you like me to suggest a small helper (e.g., a ClientHttpRequestInterceptor or ExchangeFilterFunction) that records end-to-end timings with status and error tags?
123-126: Timer already counts invocations; consider dropping @counted to avoid duplicate counters.Micrometer Timer provides count, total time, max, etc. Adding a separate Counter often duplicates cardinality and storage without adding much value unless you have a specific reason to separate them.
If you keep the Counter, add explicit tags (e.g., outcome) so it provides distinct insights vs. the Timer’s statistics.
160-163: Same note as above: redundant @counted alongside @timed.Applies here and to other similar methods (@queryAttributeInfo, @queryAsProfile).
If keeping both, document in code why the extra counter is needed (e.g., alerts based on pure request rate independent of timer histograms).
215-220: Avoid parallel stream here; keep it sequential for predictability and lower overhead.The source is a small JSON node tree; parallelism won’t help and adds overhead. Also, parallel streams can complicate MDC/trace propagation.
- boolean additionalAttributesDtrTestStyle = StreamSupport.stream(typeApiResponse + boolean additionalAttributesDtrTestStyle = StreamSupport.stream(typeApiResponse .path("content") .path("representationsAndSemantics") - .spliterator(), - true) + .spliterator(), + false)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
build.gradle(4 hunks)collector-config.yaml(1 hunks)config/application-default.properties(3 hunks)src/main/java/edu/kit/datamanager/pit/elasticsearch/PidRecordElasticWrapper.java(4 hunks)src/main/java/edu/kit/datamanager/pit/pidgeneration/PidSuffix.java(1 hunks)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java(1 hunks)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenPrefixed.java(1 hunks)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenUpperCase.java(1 hunks)src/main/java/edu/kit/datamanager/pit/pidsystem/IIdentifierSystem.java(1 hunks)src/main/java/edu/kit/datamanager/pit/pidsystem/impl/InMemoryIdentifierSystem.java(3 hunks)src/main/java/edu/kit/datamanager/pit/pidsystem/impl/handle/HandleProtocolAdapter.java(11 hunks)src/main/java/edu/kit/datamanager/pit/pidsystem/impl/local/LocalPidSystem.java(6 hunks)src/main/java/edu/kit/datamanager/pit/pitservice/impl/EmbeddedStrictValidatorStrategy.java(4 hunks)src/main/java/edu/kit/datamanager/pit/pitservice/impl/TypingService.java(1 hunks)src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java(3 hunks)src/main/java/edu/kit/datamanager/pit/typeregistry/impl/TypeApi.java(9 hunks)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/DtrTestSchemaGenerator.java(5 hunks)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/SchemaSetGenerator.java(3 hunks)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java(5 hunks)src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java(4 hunks)src/main/java/edu/kit/datamanager/pit/web/impl/TypingRESTResourceImpl.java(1 hunks)src/main/resources/log4j.properties(0 hunks)src/main/resources/log4j2.xml(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/resources/log4j.properties
🧰 Additional context used
🧬 Code Graph Analysis (13)
src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenUpperCase.java (3)
src/main/java/edu/kit/datamanager/pit/pidgeneration/PidSuffix.java (1)
PidSuffix(28-74)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenPrefixed.java (1)
Observed(29-47)
src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (6)
src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java (1)
Observed(48-115)src/main/java/edu/kit/datamanager/pit/pitservice/impl/TypingService.java (1)
Observed(49-187)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/SchemaSetGenerator.java (1)
Observed(39-87)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/DtrTestSchemaGenerator.java (1)
Observed(49-129)src/main/java/edu/kit/datamanager/pit/typeregistry/impl/TypeApi.java (1)
Observed(58-255)src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java (1)
Observed(64-115)
src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java (2)
src/main/java/edu/kit/datamanager/pit/domain/PIDRecord.java (1)
PIDRecord(34-293)src/main/java/edu/kit/datamanager/pit/domain/SimplePidRecord.java (1)
SimplePidRecord(10-60)
src/main/java/edu/kit/datamanager/pit/pitservice/impl/EmbeddedStrictValidatorStrategy.java (10)
src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenPrefixed.java (1)
Observed(29-47)src/main/java/edu/kit/datamanager/pit/elasticsearch/PidRecordElasticWrapper.java (1)
Observed(37-67)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenUpperCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java (1)
Observed(48-115)src/main/java/edu/kit/datamanager/pit/pitservice/impl/TypingService.java (1)
Observed(49-187)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/SchemaSetGenerator.java (1)
Observed(39-87)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/DtrTestSchemaGenerator.java (1)
Observed(49-129)src/main/java/edu/kit/datamanager/pit/typeregistry/impl/TypeApi.java (1)
Observed(58-255)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (1)
Observed(47-126)
src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java (1)
src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenUpperCase.java (1)
Observed(29-44)
src/main/java/edu/kit/datamanager/pit/typeregistry/schema/DtrTestSchemaGenerator.java (3)
src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/SchemaSetGenerator.java (1)
Observed(39-87)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (1)
Observed(47-126)
src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenPrefixed.java (4)
src/main/java/edu/kit/datamanager/pit/pidgeneration/PidSuffix.java (1)
PidSuffix(28-74)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenUpperCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (1)
Observed(47-126)
src/main/java/edu/kit/datamanager/pit/pitservice/impl/TypingService.java (2)
src/main/java/edu/kit/datamanager/pit/domain/Operations.java (1)
Operations(26-183)src/main/java/edu/kit/datamanager/pit/domain/PIDRecord.java (1)
PIDRecord(34-293)
src/main/java/edu/kit/datamanager/pit/pidsystem/impl/handle/HandleProtocolAdapter.java (1)
src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java (1)
Observed(48-115)
src/main/java/edu/kit/datamanager/pit/pidsystem/impl/InMemoryIdentifierSystem.java (1)
src/main/java/edu/kit/datamanager/pit/domain/PIDRecord.java (1)
PIDRecord(34-293)
src/main/java/edu/kit/datamanager/pit/elasticsearch/PidRecordElasticWrapper.java (9)
src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenPrefixed.java (1)
Observed(29-47)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenUpperCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java (1)
Observed(48-115)src/main/java/edu/kit/datamanager/pit/pitservice/impl/TypingService.java (1)
Observed(49-187)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/SchemaSetGenerator.java (1)
Observed(39-87)src/main/java/edu/kit/datamanager/pit/typeregistry/impl/TypeApi.java (1)
Observed(58-255)src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java (1)
Observed(64-115)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (1)
Observed(47-126)
src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java (5)
src/main/java/edu/kit/datamanager/pit/common/ExternalServiceException.java (1)
ExternalServiceException(7-28)src/main/java/edu/kit/datamanager/pit/common/PidNotFoundException.java (1)
PidNotFoundException(13-36)src/main/java/edu/kit/datamanager/pit/pidsystem/impl/handle/HandleBehavior.java (1)
HandleBehavior(21-122)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/DtrTestSchemaGenerator.java (1)
Observed(49-129)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (1)
Observed(47-126)
src/main/java/edu/kit/datamanager/pit/typeregistry/impl/TypeApi.java (10)
src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenPrefixed.java (1)
Observed(29-47)src/main/java/edu/kit/datamanager/pit/elasticsearch/PidRecordElasticWrapper.java (1)
Observed(37-67)src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenUpperCase.java (1)
Observed(29-44)src/main/java/edu/kit/datamanager/pit/resolver/Resolver.java (1)
Observed(48-115)src/main/java/edu/kit/datamanager/pit/pitservice/impl/TypingService.java (1)
Observed(49-187)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/SchemaSetGenerator.java (1)
Observed(39-87)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/DtrTestSchemaGenerator.java (1)
Observed(49-129)src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java (1)
Observed(64-115)src/main/java/edu/kit/datamanager/pit/typeregistry/schema/TypeApiSchemaGenerator.java (1)
Observed(47-126)
🪛 GitHub Actions: Build and Test
src/main/java/edu/kit/datamanager/pit/web/impl/TypingRESTResourceImpl.java
[error] 73-73: TypingRESTResourceImpl is not abstract and does not override abstract method createPIDs(List,boolean,WebRequest,HttpServletResponse,UriComponentsBuilder) in ITypingRestResource
[error] 105-105: createPIDs(List,boolean,WebRequest,HttpServletResponse,UriComponentsBuilder) in TypingRESTResourceImpl cannot implement createPIDs(List,boolean,WebRequest,HttpServletResponse,UriComponentsBuilder) in ITypingRestResource; return type 'ResponseEntity<List>' is not compatible with 'ResponseEntity'
[error] 101-101: method does not override or implement a method from a supertype
🔇 Additional comments (16)
build.gradle (3)
5-5: LGTM! Spring Boot upgrade looks good.The upgrade from 3.5.0 to 3.5.4 is a patch version update that should maintain backward compatibility while providing bug fixes and security updates.
53-59: LGTM! OpenTelemetry BOM import properly configured.The OpenTelemetry Instrumentation BOM is correctly imported with version management, ensuring consistent versions across all OpenTelemetry dependencies.
99-107: Verify OpenTelemetry Samplers version compatibility.Summary (concise):
- Maven Central metadata shows opentelemetry-samplers only has -alpha releases; latest listed is 1.48.0-alpha (no non-alpha/stable release).
- opentelemetry-samplers 1.47.0-alpha is compiled against opentelemetry-sdk 1.51.0; the instrumentation BOM 2.18.1 pulls core OTel artifacts around 1.52.0 — a one‑minor‑version gap is usually compatible, but opentelemetry-samplers is still an alpha artifact so risk remains.
Location to check:
- build.gradle (lines ~99–107): implementation "io.opentelemetry.contrib:opentelemetry-samplers:1.48.0-alpha"
Recommended actions (pick one):
- Keep the explicit 1.48.0-alpha pin but run full integration tests to confirm runtime compatibility.
- If you require only stable artifacts, remove/replace this dependency or wait for a non-alpha samplers release.
- If you want strict alignment, update samplers only when a build compiled against the BOM’s SDK (~1.52.x) is available.
src/main/java/edu/kit/datamanager/pit/pitservice/impl/TypingService.java (1)
49-51: LGTM on class-level observationAdding @observed at class level is consistent with the broader observability rollout.
config/application-default.properties (1)
338-345: OTLP logging properties are supported in Spring Boot 3.5.x — ensure you add an OTLP exporter/appenderVerified: Spring Boot 3.5.x exposes management.otlp.logging.* (e.g. management.otlp.logging.endpoint), so the properties in config/application-default.properties (lines 338–345) are valid.
Please note:
- File to check: config/application-default.properties — lines 338–345 (management.otlp.logging.* and related logging settings).
- Action required: Spring Boot does NOT bundle the OpenTelemetry logging exporter/appender itself — add the appropriate OTLP exporter or configure the Logback/Log4j2 OTEL appender so logs are actually sent. Optionally confirm presence of management.otlp.logging.export.enabled in your exact 3.5.x point release or use management.logging.export.enabled if you prefer a global toggle.
Likely an incorrect or invalid review comment.
src/main/java/edu/kit/datamanager/pit/pidsystem/impl/local/LocalPidSystem.java (1)
64-69: LGTM on class-level observation and explicit prefix constantInstrumentation is consistent and having a clear, centralized prefix improves readability.
src/main/java/edu/kit/datamanager/pit/elasticsearch/PidRecordElasticWrapper.java (3)
53-66: Constructor span and logging cleanup look goodUsing @WithSpan on the constructor and replacing printStackTrace with structured logging improves observability and log hygiene.
58-59: Verify default read audience "anonymousUser"Adding "anonymousUser" by default changes visibility semantics. If this field controls read access, this may unintentionally expose records.
Please confirm this is intended for all indices and environments. If not, consider:
- Making it configurable (default off in production),
- Or setting based on a policy injected into the wrapper.
37-44: Class-level @observed is consistent with the PR’s instrumentation approachNo issues spotted; annotation adds useful context across operations.
src/main/java/edu/kit/datamanager/pit/web/impl/TypingRESTResourceImpl.java (2)
203-223: generatePIDMapping: mapping key collisions on blank internal PIDsWhen the user-supplied PID is blank, the map key
""is reused (Line 221), overwriting previous entries. If these mappings are ever used to resolve references, this can lead to incorrect replacements.
- If blank internal PIDs are not meant to be referenced by other records, consider skipping them in the map.
- Otherwise, generate unique temporary keys for blanks (e.g.,
"_auto_" + index).I can provide a targeted patch if you confirm the desired behavior for blank internal PIDs.
439-456: Resolver.getRecord: ensure resolver never returns nullIf
resolver.resolve(pid)could return null,saveToElasticandquotedEtagwould NPE. GivenResolver.resolvethrows PidNotFoundException on not found, this is probably safe, but verify.Please confirm
Resolver.resolvecannot return null for existent code paths. If unsure, add a null-check returning 404.src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenLowerCase.java (1)
29-41: LGTM: clean, instrumented wrapperFinal field, class-level @observed, and a simple
@WithSpanon generate() are all appropriate. Mirrors the UpperCase variant.src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenPrefixed.java (1)
32-38: Good immutability change on fieldsMaking
generatorandprefixfinal improves thread-safety and intentions. LGTM.src/main/java/edu/kit/datamanager/pit/pidgeneration/generators/PidSuffixGenUpperCase.java (1)
38-42: LGTM: focused responsibility and immutabilityThe class cleanly uppercases the delegated suffix and returns a new PidSuffix. Immutability of
generatoris a good improvement. Matches the pattern used by the lowercase variant.src/main/java/edu/kit/datamanager/pit/typeregistry/schema/SchemaSetGenerator.java (1)
84-85: High-cardinality span attribute: confirm acceptability of exporting attributePid.@SpanAttribute on attributePid will attach a unique value per PID to spans. This is fine for tracing, but ensure your exporter/back-end cost and PII policy permit this.
Do you want me to scan the codebase to identify other parameters tagged as @SpanAttribute that might carry user data or PII?
src/main/java/edu/kit/datamanager/pit/typeregistry/impl/TypeApi.java (1)
239-249: Cache key cardinality and span attributesqueryAttributeInfo and queryAsProfile both tag profilePid/attributePid as @SpanAttribute. High cardinality is fine for traces but avoid using them as metric tags. Verify your observation-to-trace bridge isn’t promoting them to metric dimensions.
I can generate a quick script to grep for Observations with keyValueResolvers that include arguments as low-card tags if you want to be sure.
| otel.instrumentation.logback-appender.experimental.capture-mdc-attributes=trace_id,span_id | ||
| # Enable logging context propagation | ||
| logging.context.enabled=true |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Logback-specific OTEL property with Log4j2 migration: likely ineffective
You use Log4j2 per PR context, but configure:
otel.instrumentation.logback-appender.experimental.capture-mdc-attributes=trace_id,span_id
This property targets the logback appender and won’t affect Log4j2.
Please verify the correct property for Log4j2 appender. If using OpenTelemetry’s Log4j2 appender, you likely need the corresponding log4j2 property key (for example, otel.instrumentation.log4j-appender... or otel.instrumentation.log4j2-appender...). Adjust accordingly or configure MDC mapping directly in log4j2.xml.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="config/application-default.properties"
echo "=== File snippet (lines 330-360) ==="
if [ -f "$FILE" ]; then
sed -n '330,360p' "$FILE" || true
else
echo "$FILE not found"
fi
echo
echo "=== Search: otel.instrumentation keys ==="
rg -n --hidden -S 'otel.instrumentation' || true
echo
echo "=== Search: mentions of logback / log4j / log4j2 ==="
rg -n --hidden -S '\b(logback|log4j2?|log4j)\b' || true
echo
echo "=== Search: common logging config filenames ==="
rg -n --hidden -S 'log4j2\.xml|logback.*\.xml|logback-spring\.xml' || trueLength of output: 5124
🌐 Web query:
OpenTelemetry Java instrumentation log4j2 appender property "capture-mdc-attributes" or "capture-mdc" otel.instrumentation.log4j2-appender
💡 Result:
The correct configuration property for capturing MDC attributes with the OpenTelemetry Java agent Log4j2 instrumentation was renamed for consistency starting in version 1.31.0.
Property Names for Capturing MDC/Context Data
-
Prior to 1.31.0: The property was
otel.instrumentation.log4j-appender.experimental.capture-context-data-attributes[2][3]. -
Starting with 1.31.0 / Java Instrumentation 2.0:
The property has been standardized as
otel.instrumentation.log4j-appender.experimental.capture-mdc-attributes[2].
This change aligns Log4j2 instrumentation with other loggers (Log4j1, Logback, JBoss LogManager), which all use the capture-mdc-attributes naming convention[2]. -
No official property called
capture-mdc(without "-attributes") is referenced in the OpenTelemetry configuration documentation or code for Java Log4j2 instrumentation[2][3].
Example Usage
To capture MDC (Mapped Diagnostic Context) attributes with Log4j2 and the OpenTelemetry Java agent, set the following system property:
otel.instrumentation.log4j-appender.experimental.capture-mdc-attributes=trueSet this via the -D switch to the JVM, for example:
java -Dotel.instrumentation.log4j-appender.experimental.capture-mdc-attributes=true -javaagent:opentelemetry-javaagent.jar -jar yourapp.jaror set it in environment variables if supported[2][3][6].
Context
- MDC in Log4j2 is called ThreadContext (API:
org.apache.logging.log4j.ThreadContext) but is commonly referred to as MDC for compatibility and familiarity[5]. - The Log4j2 OpenTelemetry appender captures these context values if the correct property is set.
- The MDC/context data captured will be included as log record attributes in OpenTelemetry log exports[4][7].
Summary Table
| Logger | Property to Capture MDC Attributes |
|---|---|
| Log4j 1 | otel.instrumentation.log4j-appender.experimental.capture-mdc-attributes |
| Log4j 2 | otel.instrumentation.log4j-appender.experimental.capture-mdc-attributes |
| Logback | otel.instrumentation.logback-appender.experimental.capture-mdc-attributes |
| JBoss LogMgr | otel.instrumentation.jboss-logmanager.experimental.capture-mdc-attributes |
Additional Details
- If you use a property like
otel.instrumentation.log4j2-appender.experimental.capture-mdc-attributes, it should be recognized post-renaming as above.
Always confirm with your actual OpenTelemetry Java agent version documentation for accuracy, considering ongoing upstream changes[2][6]. - "capture-mdc" alone is not an official property; always use the "-attributes" version[2][3].
References:
[2]: Configuration change tracked and merged in open-telemetry/opentelemetry-java-instrumentation#9758
[3]: Usage examples and confirmation of configuration naming conventions
[4][7]: Example Log4j2 OpenTelemetry appender integration
[5]: Explanation of MDC/ThreadContext usage in Java logging frameworks.
Citations:
- 1: https://opentelemetry.io/docs/specs/otel/logs/supplementary-guidelines/
- 2: log4j MDC attribute capture configuration open-telemetry/opentelemetry-java-instrumentation#9758
- 3: Export all MDC contextual fields in logs when using otlp exporter open-telemetry/opentelemetry-java-instrumentation#6192
- 4: https://docs.honeycomb.io/send-data/logs/opentelemetry/sdk/java/
- 5: https://www.baeldung.com/mdc-in-log4j-2-logback
- 6: https://raw.githubusercontent.com/splunk/o11y-gdi-metadata/main/apm/splunk-otel-java/metadata.yaml
- 7: https://doc.akka.io/libraries/akka-insights/current/plugins/opentelemetry/logs.html
- 8: https://logging.apache.org/log4j/2.x/manual/layouts.html
- 9: https://docs.dynatrace.com/docs/ingest-from/opentelemetry/walkthroughs/java/java-manual
- 10: https://logging.apache.org/log4j/2.3.x/manual/appenders.html
Replace Logback OTEL property with Log4j appender property
Project uses Log4j2 (src/main/resources/log4j2.xml) but config sets a Logback-specific key — that won't affect the Log4j2 appender. Replace the Logback property with the Log4j appender property.
-
File to change:
- config/application-default.properties (around line 343)
-
Replace this line:
-
- otel.instrumentation.logback-appender.experimental.capture-mdc-attributes=trace_id,span_id
-
- otel.instrumentation.log4j-appender.experimental.capture-mdc-attributes=trace_id,span_id
-
(Keep logging.context.enabled=true unchanged.)
Note: the standardized Log4j key is otel.instrumentation.log4j-appender.experimental.capture-mdc-attributes (OpenTelemetry Java instrumentation >=1.31.0). For older agents the pre-1.31.0 name was otel.instrumentation.log4j-appender.experimental.capture-context-data-attributes.
🤖 Prompt for AI Agents
In config/application-default.properties around lines 343–345, replace the
Logback-specific property
otel.instrumentation.logback-appender.experimental.capture-mdc-attributes=trace_id,span_id
with the Log4j2 appender property
otel.instrumentation.log4j-appender.experimental.capture-mdc-attributes=trace_id,span_id
so the OpenTelemetry instrumentation captures MDC attributes for Log4j; leave
logging.context.enabled=true unchanged.
| otel.instrumentation.runtime-telemetry-java17.enabled=true | ||
| # Enable Spring WebMVC request tracing | ||
| otel.instrumentation.spring-webmvc.enabled=true | ||
| # Enable OpenTelemetry annotation support (@WithSpan) | ||
| otel.instrumentation.annotations.enabled=true | ||
| # Parent-based sampling with trace ID ratio |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify OpenTelemetry runtime telemetry and annotation enablement flags
Property names vary across OTEL agent versions. Ensure these are recognized by your stack.
🌐 Web query:
What are the correct OpenTelemetry Java agent properties to enable runtime telemetry (JVM metrics) and annotation-based instrumentation (e.g., @WithSpan) as of 2.0+?
💡 Result:
Short answer — the agent-config properties are:
-
Runtime telemetry (JVM metrics)
- System property: -Dotel.instrumentation.runtime-telemetry.enabled=true
- Env var: OTEL_INSTRUMENTATION_RUNTIME_TELEMETRY_ENABLED=true
- (Optional JFR / Java‑17 flags)
- -Dotel.instrumentation.runtime-telemetry-java17.enabled=true
- -Dotel.instrumentation.runtime-telemetry-java17.enable-all=true
- -Dotel.instrumentation.runtime-telemetry.emit-experimental-telemetry=true
- (Historically the runtime metrics artifact was renamed; see notes below). (github.com)
-
Annotation-based instrumentation (@WithSpan)
- System property: -Dotel.instrumentation.opentelemetry-instrumentation-annotations.enabled=true
- Env var: OTEL_INSTRUMENTATION_OPENTELEMETRY_INSTRUMENTATION_ANNOTATIONS_ENABLED=true
- You must also add the runtime dependency on the annotations library in your application:
- io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations (use the 2.x series that matches your agent). (wdk-docs.github.io, opentelemetry.io)
Examples (bash):
-
Enable runtime telemetry + annotations via env vars:
- export OTEL_INSTRUMENTATION_RUNTIME_TELEMETRY_ENABLED=true
- export OTEL_INSTRUMENTATION_OPENTELEMETRY_INSTRUMENTATION_ANNOTATIONS_ENABLED=true
-
Or on the java command line:
- java -javaagent:/path/opentelemetry-javaagent.jar
-Dotel.instrumentation.runtime-telemetry.enabled=true
-Dotel.instrumentation.opentelemetry-instrumentation-annotations.enabled=true
-jar myapp.jar
- java -javaagent:/path/opentelemetry-javaagent.jar
Notes / gotchas
- The runtime metrics pieces were renamed/split in the 1.x → 2.x work (artifact opentelemetry-runtime-metrics → runtime-telemetry Java8/Java17). If you are migrating from older agent versions you may see older names like "runtime-metrics" used in some docs or examples; prefer the runtime-telemetry names for 2.0+. (github.com)
- For JFR-based metrics on Java 17 you need to opt in (see the runtime-telemetry Java17 flags above). (github.com)
- If you want to enable only manual/annotation instrumentation while disabling other auto-instrumentations, the example combination used by the project is:
- -Dotel.instrumentation.common.default-enabled=false
-Dotel.instrumentation.opentelemetry-api.enabled=true
-Dotel.instrumentation.opentelemetry-instrumentation-annotations.enabled=true. (wdk-docs.github.io)
- -Dotel.instrumentation.common.default-enabled=false
If you want, tell me your agent version and Java version and I’ll give the exact -D / OTEL_ lines you should use and the annotation library coordinate to add to your build.
Citations:
- 1: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/runtime-telemetry/README.md?utm_source=chatgpt.com
- 2: https://wdk-docs.github.io/opentelemetry-docs/docs/instrumentation/java/automatic/agent-config/?utm_source=chatgpt.com
- 3: https://opentelemetry.io/docs/zero-code/java/agent/annotations/?utm_source=chatgpt.com
- 4: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/CHANGELOG.md?utm_source=chatgpt.com
- 5: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/runtime-telemetry/README.md?utm_source=chatgpt.com
- 6: https://wdk-docs.github.io/opentelemetry-docs/docs/instrumentation/java/automatic/agent-config/?utm_source=chatgpt.com
Update OpenTelemetry agent property names in config/application-default.properties
The properties in the snippet use non-standard/partial names for OTEL Java Instrumentation 2.x+. Replace them with the correct agent properties and add the annotation runtime dependency.
- Files to change:
- config/application-default.properties — lines ~361–366
Suggested replacement snippet:
# Enable runtime telemetry (JVM metrics)
otel.instrumentation.runtime-telemetry.enabled=true
# (optional: enable JFR/Java17-specific telemetry)
otel.instrumentation.runtime-telemetry-java17.enabled=true
# Enable Spring WebMVC request tracing
otel.instrumentation.spring-webmvc.enabled=true
# Enable OpenTelemetry annotation support (@WithSpan)
otel.instrumentation.opentelemetry-instrumentation-annotations.enabled=true
Additional note: Add the runtime dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations:2.x (match your agent version). If you only enabled the Java17 flag, also ensure otel.instrumentation.runtime-telemetry.enabled=true is set.
| @Override | ||
| public void validate(PIDRecord pidRecord) | ||
| throws RecordValidationException, ExternalServiceException | ||
| { | ||
| @WithSpan(kind = SpanKind.INTERNAL) | ||
| @Timed(value = "validation_embedded_strict", description = "Time taken for embedded strict validation") | ||
| @Counted(value = "validation_embedded_strict_total", description = "Total number of embedded strict validations") | ||
| public void validate(@SpanAttribute PIDRecord pidRecord) | ||
| throws RecordValidationException, ExternalServiceException { | ||
| if (pidRecord.getPropertyIdentifiers().isEmpty()) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove @SpanAttribute on PIDRecord to prevent sensitive data in traces
Same concern as elsewhere: serializing PIDRecord in span attributes risks PII leakage and high cardinality.
Apply this diff:
- private static void unpackAsyncExceptions(@SpanAttribute PIDRecord pidRecord, Throwable e) {
+ private static void unpackAsyncExceptions(PIDRecord pidRecord, Throwable e) {
@@
- public void validate(@SpanAttribute PIDRecord pidRecord)
+ public void validate(PIDRecord pidRecord)Inside spans, set only minimal attributes (e.g., pid, attribute count) as needed.
Also applies to: 75-77
🤖 Prompt for AI Agents
In
src/main/java/edu/kit/datamanager/pit/pitservice/impl/EmbeddedStrictValidatorStrategy.java
around lines 75-77 and 92-98, remove the @SpanAttribute annotation from the
PIDRecord method parameter to avoid serializing sensitive/high-cardinality data
into traces; instead, set only minimal span attributes inside the method (for
example the PID string and the property identifier count) using the tracer/span
API or annotate primitive/string getters, so replace the parameter-level
@SpanAttribute with explicit, low-cardinality attributes added within the method
body.
| @SpanAttribute boolean dryrun, | ||
| WebRequest request, | ||
| HttpServletResponse response, | ||
| UriComponentsBuilder uriBuilder | ||
| ) throws IOException, RecordValidationException, ExternalServiceException { | ||
| Instant startTime = Instant.now(); | ||
| LOG.info("Creating PIDs for {} records.", rec.size()); | ||
| String prefix = this.typingService.getPrefix().orElseThrow(() -> new IOException("No prefix configured.")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid @SpanAttribute on List; record size instead
Annotating the full list is high-cardinality and leaks content via toString. Track only batch size.
Apply this diff to remove the attribute from the parameter and set a low-cardinality attribute:
- public ResponseEntity<List<PIDRecord>> createPIDs(
- @SpanAttribute List<PIDRecord> rec,
- @SpanAttribute boolean dryrun,
+ public ResponseEntity<List<PIDRecord>> createPIDs(
+ List<PIDRecord> rec,
+ @SpanAttribute boolean dryrun,
WebRequest request,
HttpServletResponse response,
UriComponentsBuilder uriBuilder
) throws IOException, RecordValidationException, ExternalServiceException {
- Instant startTime = Instant.now();
+ io.opentelemetry.api.trace.Span.current().setAttribute("pit.batch.size", rec != null ? rec.size() : 0);
+ Instant startTime = Instant.now();Note: Make sure io.opentelemetry.api:opentelemetry-api is available (it is in this PR).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @SpanAttribute List<PIDRecord> rec, | |
| @SpanAttribute boolean dryrun, | |
| WebRequest request, | |
| HttpServletResponse response, | |
| UriComponentsBuilder uriBuilder | |
| ) throws IOException, RecordValidationException, ExternalServiceException { | |
| Instant startTime = Instant.now(); | |
| LOG.info("Creating PIDs for {} records.", rec.size()); | |
| List<PIDRecord> rec, | |
| @SpanAttribute boolean dryrun, | |
| WebRequest request, | |
| HttpServletResponse response, | |
| UriComponentsBuilder uriBuilder | |
| ) throws IOException, RecordValidationException, ExternalServiceException { | |
| io.opentelemetry.api.trace.Span.current().setAttribute("pit.batch.size", rec != null ? rec.size() : 0); | |
| Instant startTime = Instant.now(); | |
| LOG.info("Creating PIDs for {} records.", rec.size()); |
🤖 Prompt for AI Agents
In src/main/java/edu/kit/datamanager/pit/web/impl/TypingRESTResourceImpl.java
around lines 106 to 113, remove the @SpanAttribute annotation from the
List<PIDRecord> parameter (to avoid high-cardinality/leaky toString) and instead
set a low-cardinality span attribute with the batch size inside the method body,
e.g. call Span.current().setAttribute("pid.batch.size", rec.size()) (ensure
io.opentelemetry.api:opentelemetry-api is available), leaving the rest of the
method signature intact.
| try { | ||
| // register the PID | ||
| String pid = this.typingService.registerPid(pidRecord); | ||
| pidRecord.setPid(pid); | ||
|
|
||
| // store pid locally in accordance with the storage strategy | ||
| if (applicationProps.getStorageStrategy().storesModified()) { | ||
| storeLocally(pid, true); | ||
| } | ||
|
|
||
| // distribute pid creation event to other services | ||
| PidRecordMessage message = PidRecordMessage.creation( | ||
| pid, | ||
| "", // TODO parameter is deprecated and will be removed soon. | ||
| AuthenticationHelper.getPrincipal(), | ||
| ControllerUtils.getLocalHostname()); | ||
| try { | ||
| this.messagingService.send(message); | ||
| } catch (Exception e) { | ||
| LOG.error("Could not notify messaging service about the following message: {}", message); | ||
| } | ||
|
|
||
| // save the record to elastic | ||
| this.saveToElastic(pidRecord); | ||
| } catch (Exception e) { | ||
| LOG.error("Could not register PID for record {}. Error: {}", pidRecord, e.getMessage()); | ||
| failedRecords.add(pidRecord); | ||
| validatedRecords.remove(pidRecord); | ||
| } | ||
| }); | ||
|
|
||
| Instant endTime = Instant.now(); |
There was a problem hiding this comment.
ConcurrentModificationException risk: mutating list during forEach
Calling validatedRecords.remove(pidRecord) inside validatedRecords.forEach(...) is unsafe and can throw ConcurrentModificationException.
Replace with separate success/failure lists and avoid mutating while iterating:
- List<PIDRecord> failedRecords = new ArrayList<>();
- // register the records
- validatedRecords.forEach(pidRecord -> {
- try {
- // register the PID
- String pid = this.typingService.registerPid(pidRecord);
- pidRecord.setPid(pid);
- // store pid locally in accordance with the storage strategy
- if (applicationProps.getStorageStrategy().storesModified()) {
- storeLocally(pid, true);
- }
- // distribute pid creation event to other services
- PidRecordMessage message = PidRecordMessage.creation(
- pid,
- "", // TODO parameter is deprecated and will be removed soon.
- AuthenticationHelper.getPrincipal(),
- ControllerUtils.getLocalHostname());
- try {
- this.messagingService.send(message);
- } catch (Exception e) {
- LOG.error("Could not notify messaging service about the following message: {}", message);
- }
- // save the record to elastic
- this.saveToElastic(pidRecord);
- } catch (Exception e) {
- LOG.error("Could not register PID for record {}. Error: {}", pidRecord, e.getMessage());
- failedRecords.add(pidRecord);
- validatedRecords.remove(pidRecord);
- }
- });
+ List<PIDRecord> failedRecords = new ArrayList<>();
+ List<PIDRecord> successfulRecords = new ArrayList<>();
+ for (PIDRecord pidRecord : validatedRecords) {
+ try {
+ // register the PID
+ String pid = this.typingService.registerPid(pidRecord);
+ pidRecord.setPid(pid);
+ // store pid locally in accordance with the storage strategy
+ if (applicationProps.getStorageStrategy().storesModified()) {
+ storeLocally(pid, true);
+ }
+ // distribute pid creation event to other services
+ PidRecordMessage message = PidRecordMessage.creation(
+ pid,
+ "", // TODO parameter is deprecated and will be removed soon.
+ AuthenticationHelper.getPrincipal(),
+ ControllerUtils.getLocalHostname());
+ try {
+ this.messagingService.send(message);
+ } catch (Exception e) {
+ LOG.error("Could not notify messaging service about the following message: {}", message);
+ }
+ // save the record to elastic
+ this.saveToElastic(pidRecord);
+ successfulRecords.add(pidRecord);
+ } catch (Exception e) {
+ LOG.error("Could not register PID for record {}. Error: {}", pidRecord, e.getMessage());
+ failedRecords.add(pidRecord);
+ }
+ }
@@
- if (!failedRecords.isEmpty()) {
- for (PIDRecord successfulRecord : validatedRecords) { // rollback the successful records
+ if (!failedRecords.isEmpty()) {
+ for (PIDRecord successfulRecord : successfulRecords) { // rollback the successful records
try {
LOG.debug("Rolling back PID creation for record with PID {}.", successfulRecord.getPid());
this.typingService.deletePid(successfulRecord.getPid());
} catch (Exception e) {
LOG.error("Could not rollback PID creation for record with PID {}. Error: {}", successfulRecord.getPid(), e.getMessage());
}
}
@@
- LOG.info("Creation finished. Returning validated records for {} records.", validatedRecords.size());
- return ResponseEntity.status(HttpStatus.CREATED).body(validatedRecords);
+ LOG.info("Creation finished. Returning validated records for {} records.", successfulRecords.size());
+ return ResponseEntity.status(HttpStatus.CREATED).body(successfulRecords);Also applies to: 175-190
| Optional<PidSuffix> maybeSuffix = Streams.failableStream(suffixStream) | ||
| // With failable streams, we can throw exceptions. | ||
| .filter(suffix -> !this.typingService.isPidRegistered(suffix)) | ||
| .stream() // back to normal java streams | ||
| .findFirst(); // as the stream is infinite, we should always find a prefix. | ||
| PidSuffix suffix = maybeSuffix | ||
| .orElseThrow(() -> new ExternalServiceException("Could not generate PID suffix which did not exist yet.")); | ||
| pidRecord.setPid(suffix.get()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Type mismatch: isPidRegistered called with PidSuffix instead of String, and uniqueness check must be full PID (prefix + suffix)
typingService.isPidRegistered is defined for String PIDs. Also, uniqueness must be checked against the full PID including the prefix.
Fix by checking prefix + suffix.get() and use a proper ExternalServiceException factory:
- Stream<PidSuffix> suffixStream = suffixGenerator.infiniteStream();
- Optional<PidSuffix> maybeSuffix = Streams.failableStream(suffixStream)
- // With failable streams, we can throw exceptions.
- .filter(suffix -> !this.typingService.isPidRegistered(suffix))
- .stream() // back to normal java streams
- .findFirst(); // as the stream is infinite, we should always find a prefix.
+ final String prefix = this.typingService.getPrefix()
+ .orElseThrow(() -> new InvalidConfigException("No prefix configured."));
+ Stream<PidSuffix> suffixStream = suffixGenerator.infiniteStream();
+ Optional<PidSuffix> maybeSuffix = Streams.failableStream(suffixStream)
+ .filter(suffix -> !this.typingService.isPidRegistered(prefix + suffix.get()))
+ .stream()
+ .findFirst(); // as the stream is infinite, we should always find a suffix.
PidSuffix suffix = maybeSuffix
- .orElseThrow(() -> new ExternalServiceException("Could not generate PID suffix which did not exist yet."));
+ .orElseThrow(() -> new ExternalServiceException("PID Generation", "Could not generate a unique PID suffix."));
pidRecord.setPid(suffix.get());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Stream<PidSuffix> suffixStream = suffixGenerator.infiniteStream(); | |
| Optional<PidSuffix> maybeSuffix = Streams.failableStream(suffixStream) | |
| // With failable streams, we can throw exceptions. | |
| .filter(suffix -> !this.typingService.isPidRegistered(suffix)) | |
| .stream() // back to normal java streams | |
| .findFirst(); // as the stream is infinite, we should always find a prefix. | |
| PidSuffix suffix = maybeSuffix | |
| .orElseThrow(() -> new ExternalServiceException("Could not generate PID suffix which did not exist yet.")); | |
| pidRecord.setPid(suffix.get()); | |
| } | |
| final String prefix = this.typingService.getPrefix() | |
| .orElseThrow(() -> new InvalidConfigException("No prefix configured.")); | |
| Stream<PidSuffix> suffixStream = suffixGenerator.infiniteStream(); | |
| Optional<PidSuffix> maybeSuffix = Streams.failableStream(suffixStream) | |
| .filter(suffix -> !this.typingService.isPidRegistered(prefix + suffix.get())) | |
| .stream() | |
| .findFirst(); // as the stream is infinite, we should always find a suffix. | |
| PidSuffix suffix = maybeSuffix | |
| .orElseThrow(() -> new ExternalServiceException("PID Generation", "Could not generate a unique PID suffix.")); | |
| pidRecord.setPid(suffix.get()); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/edu/kit/datamanager/pit/web/impl/TypingRESTResourceImpl.java
around lines 333 to 342, the code calls typingService.isPidRegistered with a
PidSuffix and only checks the suffix, but the service expects a full PID String
and uniqueness must be checked against prefix+suffix; update the filter to
compute the candidatePid as prefix + suffix.get() and call
typingService.isPidRegistered(candidatePid) instead of passing the PidSuffix,
and when no suffix is found throw the ExternalServiceException using the proper
factory/constructor used across the project (i.e. create the exception via the
standard ExternalServiceException factory method/constructor with a clear
message).
| return new PageImpl<>(intersection); | ||
| } else if (queriesCreated) { | ||
| return resultCreatedTimestamp; |
There was a problem hiding this comment.
Spring Page has no filter method
Page<T> does not expose a filter method. This will not compile. Use streams on the content.
Replace with:
- final Page<KnownPid> tmp = resultModifiedTimestamp;
- final List<KnownPid> intersection = resultCreatedTimestamp.filter((x) -> tmp.getContent().contains(x)).toList();
- return new PageImpl<>(intersection);
+ final List<KnownPid> intersection = resultCreatedTimestamp.getContent().stream()
+ .filter(resultModifiedTimestamp.getContent()::contains)
+ .toList();
+ return new PageImpl<>(intersection);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final List<KnownPid> intersection = resultCreatedTimestamp.filter((x) -> tmp.getContent().contains(x)).toList(); | |
| return new PageImpl<>(intersection); | |
| } else if (queriesCreated) { | |
| final List<KnownPid> intersection = resultCreatedTimestamp.getContent().stream() | |
| .filter(resultModifiedTimestamp.getContent()::contains) | |
| .toList(); | |
| return new PageImpl<>(intersection); | |
| } else if (queriesCreated) { |
🤖 Prompt for AI Agents
In src/main/java/edu/kit/datamanager/pit/web/impl/TypingRESTResourceImpl.java
around lines 521-523, the code calls filter(...) on a Spring Page which doesn't
exist; replace that with a stream over the page content: call
resultCreatedTimestamp.getContent().stream().filter(x ->
tmp.getContent().contains(x)).collect(Collectors.toList()) to produce the
intersection list, then construct and return a new PageImpl<>(intersection) (or
new PageImpl<>(intersection, resultCreatedTimestamp.getPageable(),
intersection.size()/use original total if needed) if you need to preserve paging
metadata).
| <Root> | ||
| <AppenderRef ref="OpenTelemetryAppender" level="All"/> | ||
| </Root> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adjusting root logger level for production.
Setting the root logger to level "All" may produce excessive logging in production, impacting performance and making it difficult to identify important messages.
Consider using environment-specific logging levels:
<Loggers>
<Root>
- <AppenderRef ref="OpenTelemetryAppender" level="All"/>
+ <AppenderRef ref="OpenTelemetryAppender" level="${env:LOG_LEVEL:-INFO}"/>
</Root>
</Loggers>This allows configuration via environment variable with INFO as default.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Root> | |
| <AppenderRef ref="OpenTelemetryAppender" level="All"/> | |
| </Root> | |
| <Root> | |
| <AppenderRef ref="OpenTelemetryAppender" level="${env:LOG_LEVEL:-INFO}"/> | |
| </Root> |
🤖 Prompt for AI Agents
In src/main/resources/log4j2.xml around lines 7 to 9, the Root logger is set to
level "All" which is too verbose for production; change the Root logger to use
an environment-configurable level with a safe default (e.g., use an env var like
LOG_LEVEL with default INFO) and remove the hardcoded "All" level on the
AppenderRef so logging level is controlled by the Root logger via property
substitution; ensure the config documents the env var and that the default is
INFO for production safety.
Signed-off-by: Maximilian Inckmann <[email protected]>
Signed-off-by: Maximilian Inckmann <[email protected]>
minor fixes Signed-off-by: Maximilian Inckmann <[email protected]>
Pfeil
left a comment
Pfeil
left a comment
There was a problem hiding this comment.
Short first look at the config file.
| #logging.level.root: ERROR | ||
| #logging.level.edu.kit.datamanager.doip:TRACE | ||
| logging.level.edu.kit=WARN | ||
| logging.level.edu.kit=INFO |
There was a problem hiding this comment.
Why? Is this somehow required for any feature?
| # When to include the "message" attribute in HTTP responses on uncatched exceptions. | ||
| spring.application.name=typed-pid-maker | ||
| #spring.profiles.active=default | ||
| server.error.include-message=always |
There was a problem hiding this comment.
Split another property from its comment.
There was a problem hiding this comment.
None of these changes have been applied to the docker config. Check where else they will apply.
| ############################### | ||
|
|
||
| spring.autoconfigure.exclude=org.keycloak.adapters.springboot.KeycloakAutoConfiguration,org.springframework.boot.autoconfigure.security.servlet.SecurityAutoConfiguration |
There was a problem hiding this comment.
TODO undo removing the empty lines and ensure the file is readable.
|
|
||
| ################################ | ||
| ####### Observability ########## | ||
| ################################ | ||
|
|
There was a problem hiding this comment.
Documentation missing. This is far away from being self-explanatory.
| ### 5) Sensitive Information Configuration (COMMENTED OUT FOR PRODUCTION) ### | ||
| ## WARNING: These capture potentially sensitive data - NOT for production | ||
| ## Captures HTTP request headers (may contain auth tokens) |
There was a problem hiding this comment.
Why should I capture this even in development environments? We have debugging, and loggers would at least not send the information around.
| ## Include PII (Personally Identifiable Information) as trace attributes | ||
| pit.observability.includePiiInTraces=true | ||
|
|
3 participants
In this PR, I realized OpenTelemetry metrics, logging, and tracing for the Typed PID Maker. I added annotations for metrics that support timing (including histograms) and counting. I purely used annotations and no vendor-specific APIs (only OpenTelemetry). Please check whether all the metrics are needed
Summary by CodeRabbit