Java Metrics SDK implementation (Spring Boot) #1179
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erunion
added
enhancement
mosesj-readme
requested changes
Mar 11, 2025
Comment on lines
+30
to
+31
| //Uncomment the code below to have a custom user data collection configuration. | ||
| //It automatically overrides the default one |
| .digest(apiKey.getBytes(StandardCharsets.UTF_8))); | ||
|
|
||
| String last4Digits = apiKey.substring(apiKey.length() - 4); | ||
| return "sha512-" + base64Hash + "?" + last4Digits; |
There was a problem hiding this comment.
is there a reason to use the encoding algo in the masked key value?
| @@ -0,0 +1,19 @@ | |||
| package com.readme.core.dataextraction; | |||
|
|
|||
| import lombok.*; | |||
There was a problem hiding this comment.
just a note, can we update the imports to only be what we need. This will increase compiled resource because we're importing things we don't need.
Comment on lines
+11
to
+17
| List<String> allowlist; | ||
| List<String> denylist; | ||
| boolean development; | ||
| boolean fireAndForget; | ||
| String baseLogUrl; | ||
| @Builder.Default | ||
| int bufferLength = 1; |
There was a problem hiding this comment.
can you add comments explaining what these properties are. Some are self explanatory, but some aren't. Also for our team Java is not their native programming language, so providing more context for them will make future improvements and readability better.
Consider this a comment for all of the builders in this PR. Thank you.
| List<String> denylist; | ||
| boolean development; | ||
| boolean fireAndForget; | ||
| String baseLogUrl; |
There was a problem hiding this comment.
is this allowed to be null? If not hould this be marked as @nonnull
| private static String getRequestParametersAsString(Map<String, String> requestParameters) { | ||
| return requestParameters.entrySet() | ||
| .stream() | ||
| .map(entry -> entry.getKey() + "=" + entry.getValue()) |
There was a problem hiding this comment.
does this need a null check for the entry value? is null always an acceptable value for the possible query params?
| try { | ||
| OutgoingLogBody outgoingLogBody = payloadConstructor.construct(payloadData, logOptions); | ||
| buffer.add(outgoingLogBody); | ||
| if (buffer.size() >= logOptions.getBufferLength()) { |
There was a problem hiding this comment.
what happens if an operation completes and the buffer isn't over the size limit? How are the logs dispatched then?
| @JsonAnySetter | ||
| public void setAdditionalField(String key, Object value) { | ||
| if (additional == null) { | ||
| additional = new HashMap<>(8); |
There was a problem hiding this comment.
can you
- change the
8to a constant. Magic numbers aren't self documenting - it seems that many of the classes in this package use the same value to limit the
additionalHashMap size. Can you extract that so it can be used in both places from the same constant. Assuming they are supposed to have the same value.
Comment on lines
+25
to
+26
| //TODO Fix: | ||
| // Handle Basic tokens as well as Bearer ones |
There was a problem hiding this comment.
not seeing where this is being handled
Comment on lines
+35
to
+54
| public String extractFromBody(ServletDataPayloadAdapter payload, String fieldPath) { | ||
| if (!payload.getRequestMethod().equalsIgnoreCase(HttpMethod.GET.name())) { | ||
| if (payload.getRequestContentType().equalsIgnoreCase("application/json")) { | ||
| String requestBody = payload.getRequestBody(); | ||
| try { | ||
| JsonNode currentNode = objectMapper.readTree(requestBody); | ||
| if (!fieldPath.startsWith("/")) { | ||
| fieldPath = "/" + fieldPath; | ||
| } | ||
| return currentNode.at(fieldPath).asText(); | ||
| } catch (Exception e) { | ||
| log.error("Error when reading the user data from JSON body: {}", e.getMessage()); | ||
| } | ||
| } | ||
| log.error("The provided body content type {} is not supported to get user data.", payload.getRequestContentType()); | ||
| return ""; | ||
| } | ||
| log.error("The HTTP method {} is not supported to get user data from body.", payload.getRequestMethod()); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
guard statements would make this much easier to read:
Suggested change
| public String extractFromBody(ServletDataPayloadAdapter payload, String fieldPath) { | |
| if (!payload.getRequestMethod().equalsIgnoreCase(HttpMethod.GET.name())) { | |
| if (payload.getRequestContentType().equalsIgnoreCase("application/json")) { | |
| String requestBody = payload.getRequestBody(); | |
| try { | |
| JsonNode currentNode = objectMapper.readTree(requestBody); | |
| if (!fieldPath.startsWith("/")) { | |
| fieldPath = "/" + fieldPath; | |
| } | |
| return currentNode.at(fieldPath).asText(); | |
| } catch (Exception e) { | |
| log.error("Error when reading the user data from JSON body: {}", e.getMessage()); | |
| } | |
| } | |
| log.error("The provided body content type {} is not supported to get user data.", payload.getRequestContentType()); | |
| return ""; | |
| } | |
| log.error("The HTTP method {} is not supported to get user data from body.", payload.getRequestMethod()); | |
| return ""; | |
| } | |
| public String extractFromBody(ServletDataPayloadAdapter payload, String fieldPath) { | |
| if (payload.getRequestMethod().equalsIgnoreCase(HttpMethod.GET.name())) { | |
| log.error("The HTTP method {} is not supported to get user data from body.", payload.getRequestMethod()); | |
| return ""; | |
| } | |
| if (!payload.getRequestContentType().equalsIgnoreCase("application/json")) { | |
| log.error("The provided body content type {} is not supported to get user data.", payload.getRequestContentType()); | |
| return ""; | |
| } | |
| String requestBody = payload.getRequestBody(); | |
| try { | |
| JsonNode currentNode = objectMapper.readTree(requestBody); | |
| if (!fieldPath.startsWith("/")) { | |
| fieldPath = "/" + fieldPath; | |
| } | |
| return currentNode.at(fieldPath).asText(); | |
| } catch (Exception e) { | |
| log.error("Error when reading the user data from JSON body: {}", e.getMessage()); | |
| } | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is done?
After testing it with Spring Boot 3+ and fixing bugs, we can add implementation for Spring Boot 2+. It will take about 1 working day to do required changes and test it.
