fix: [NEXT-1924] Add metrics and fix issue of description being too long in some cases#2386
Merged
Conversation
Contributor
WalkthroughIntegrates Datadog metrics observability into the notification service by adding AWS SDK and Micrometer dependencies, introducing a new PaladinMetrics utility class that retrieves Datadog credentials from AWS Secrets Manager, and instrumenting SendNotification to emit counters for invocation, success, and error events. Changes
Sequence DiagramsequenceDiagram
participant Handler as SendNotification
participant Metrics as PaladinMetrics
participant SecretsManager as AWS Secrets Manager
participant Datadog as DatadogMeterRegistry
Handler->>Metrics: initialize(tags)
Metrics->>SecretsManager: getSecret(datadog-credentials)
SecretsManager-->>Metrics: JSON credentials
Metrics->>Metrics: parseJSON & extract apiKey
Metrics->>Datadog: configure registry with apiKey & tags
Datadog-->>Metrics: ready
Handler->>Metrics: incrementCount("invoke", tags)
Metrics->>Datadog: emit counter
Handler->>Handler: sendPayload with trimDescription()
alt Success
Handler->>Metrics: incrementCount("send-success", tags)
else Error
Handler->>Metrics: incrementCount("send-error", tags)
end
Metrics->>Datadog: emit counter
Handler->>Metrics: close()
Metrics->>Datadog: shutdown registry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@lambda-functions/notification-invoke-service/src/main/java/com/paladincloud/PaladinMetrics.java`:
- Around line 25-67: The initialize method can throw (getDatadogSecret, JSON
parsing, missing keys) and close()/incrementCount() assume registry is non-null;
make metrics best-effort by catching Throwable inside initialize (around
getDatadogSecret and registry construction), log the error and leave registry
null instead of propagating; in close() guard registry with if (registry !=
null) before calling close and then set registry = null; in
incrementCount(String name, String ...tags) return early if registry is null (or
catch exceptions from registry.counter()/increment and log them) so metric
failures never throw back into the Lambda; update references to registry,
initialize, close, and incrementCount accordingly.
In
`@lambda-functions/notification-invoke-service/src/main/java/com/paladincloud/SendNotification.java`:
- Around line 82-83: The logging call uses obj.toString() which throws NPE if
obj is null; update the block around PaladinMetrics.incrementCount and
LOGGER.error to guard against null by converting obj safely (e.g., use
String.valueOf(obj) or Objects.toString(obj, "null")) and compute length only
from that safe string (or use (obj==null?0:obj.toString().length())); replace
direct obj.toString() usages in the LOGGER.error call with the safe
representation to avoid NPE in SendNotification.
- Around line 102-117: The trimDescription method currently casts and calls Gson
methods unsafely; wrap the initial parse in a try-catch (catch
JsonSyntaxException/RuntimeException) and return the original jsonStr on any
failure, and replace direct casts with safe checks: after parsing check
JsonElement.isJsonObject() before calling getAsJsonObject() (refer to JsonParser
and jo), check that jo.get("payload") exists and isJsonObject() before using
payloadObj, and verify descObj.isJsonPrimitive() and
getAsJsonPrimitive().isString() before calling getAsString(); only then truncate
using maxDescriptionLength and LOGGER.warn, otherwise return jsonStr unchanged.
Comment on lines
+25
to
+67
| public static void initialize(String ...tags) { | ||
| JsonObject secret = getDatadogSecret(); | ||
|
|
||
| DatadogConfig config = new DatadogConfig() { | ||
| @Override | ||
| public String apiKey() { | ||
| return secret.get("apiKey").getAsString(); | ||
| } | ||
|
|
||
| @Override | ||
| public String applicationKey() { | ||
| return secret.get("applicationKey").getAsString(); | ||
| } | ||
|
|
||
| @Override | ||
| public Duration step() { | ||
| return Duration.ofSeconds(10); | ||
| } | ||
|
|
||
| @Override | ||
| public String get(String k) { | ||
| return null; | ||
| } | ||
| }; | ||
|
|
||
| registry = new DatadogMeterRegistry(config, Clock.SYSTEM); | ||
|
|
||
| Tags commonTags = Tags.of(tags); | ||
| if (secret.get("env") != null) { | ||
| commonTags = commonTags.and("env", secret.get("env").getAsString()); | ||
| } | ||
| registry.config().commonTags(commonTags); | ||
| } | ||
|
|
||
| public static void close() { | ||
| registry.close(); | ||
| registry = null; | ||
| } | ||
|
|
||
| public static void incrementCount(String name, String ...tags) { | ||
| Counter counter = registry.counter(name, tags); | ||
| counter.increment(); | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent metrics failures from breaking the notification flow.
initialize() can throw (Secrets Manager, JSON parsing, missing keys), and close()/incrementCount() assume registry is non‑null. This can fail the Lambda or throw NPEs, even though metrics should be best‑effort.
🐛 Proposed fix (best‑effort metrics with null guards)
public static void initialize(String ...tags) {
- JsonObject secret = getDatadogSecret();
-
- DatadogConfig config = new DatadogConfig() {
- `@Override`
- public String apiKey() {
- return secret.get("apiKey").getAsString();
- }
- `@Override`
- public String applicationKey() {
- return secret.get("applicationKey").getAsString();
- }
- `@Override`
- public Duration step() {
- return Duration.ofSeconds(10);
- }
- `@Override`
- public String get(String k) {
- return null;
- }
- };
-
- registry = new DatadogMeterRegistry(config, Clock.SYSTEM);
-
- Tags commonTags = Tags.of(tags);
- if (secret.get("env") != null) {
- commonTags = commonTags.and("env", secret.get("env").getAsString());
- }
- registry.config().commonTags(commonTags);
+ try {
+ JsonObject secret = getDatadogSecret();
+ DatadogConfig config = new DatadogConfig() {
+ `@Override`
+ public String apiKey() { return secret.get("apiKey").getAsString(); }
+ `@Override`
+ public String applicationKey() { return secret.get("applicationKey").getAsString(); }
+ `@Override`
+ public Duration step() { return Duration.ofSeconds(10); }
+ `@Override`
+ public String get(String k) { return null; }
+ };
+
+ DatadogMeterRegistry newRegistry = new DatadogMeterRegistry(config, Clock.SYSTEM);
+ Tags commonTags = Tags.of(tags);
+ if (secret.has("env") && !secret.get("env").isJsonNull()) {
+ commonTags = commonTags.and("env", secret.get("env").getAsString());
+ }
+ newRegistry.config().commonTags(commonTags);
+ registry = newRegistry;
+ } catch (RuntimeException ex) {
+ logger.error("Metrics initialization failed; continuing without metrics.", ex);
+ registry = null;
+ }
}
public static void close() {
- registry.close();
- registry = null;
+ if (registry != null) {
+ registry.close();
+ registry = null;
+ }
}
public static void incrementCount(String name, String ...tags) {
- Counter counter = registry.counter(name, tags);
- counter.increment();
+ if (registry == null) {
+ return;
+ }
+ Counter counter = registry.counter(name, tags);
+ counter.increment();
}🤖 Prompt for AI Agents
In
`@lambda-functions/notification-invoke-service/src/main/java/com/paladincloud/PaladinMetrics.java`
around lines 25 - 67, The initialize method can throw (getDatadogSecret, JSON
parsing, missing keys) and close()/incrementCount() assume registry is non-null;
make metrics best-effort by catching Throwable inside initialize (around
getDatadogSecret and registry construction), log the error and leave registry
null instead of propagating; in close() guard registry with if (registry !=
null) before calling close and then set registry = null; in
incrementCount(String name, String ...tags) return early if registry is null (or
catch exceptions from registry.counter()/increment and log them) so metric
failures never throw back into the Lambda; update references to registry,
initialize, close, and incrementCount accordingly.
Comment on lines
+82
to
+83
| PaladinMetrics.incrementCount(NOTIFICATION_INVOKE_SVC + "-send-error"); | ||
| LOGGER.error("Failed sending message: length={}; {}", obj.toString().length(), obj, ex); |
Contributor
There was a problem hiding this comment.
Avoid NPE when logging failed list items.
obj.toString() will throw if the list contains a null element, masking the real error.
💡 Suggested fix
- LOGGER.error("Failed sending message: length={}; {}", obj.toString().length(), obj, ex);
+ String objStr = String.valueOf(obj);
+ LOGGER.error("Failed sending message: length={}; {}", objStr.length(), objStr, ex);📝 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.
Suggested change
| PaladinMetrics.incrementCount(NOTIFICATION_INVOKE_SVC + "-send-error"); | |
| LOGGER.error("Failed sending message: length={}; {}", obj.toString().length(), obj, ex); | |
| PaladinMetrics.incrementCount(NOTIFICATION_INVOKE_SVC + "-send-error"); | |
| String objStr = String.valueOf(obj); | |
| LOGGER.error("Failed sending message: length={}; {}", objStr.length(), objStr, ex); |
🤖 Prompt for AI Agents
In
`@lambda-functions/notification-invoke-service/src/main/java/com/paladincloud/SendNotification.java`
around lines 82 - 83, The logging call uses obj.toString() which throws NPE if
obj is null; update the block around PaladinMetrics.incrementCount and
LOGGER.error to guard against null by converting obj safely (e.g., use
String.valueOf(obj) or Objects.toString(obj, "null")) and compute length only
from that safe string (or use (obj==null?0:obj.toString().length())); replace
direct obj.toString() usages in the LOGGER.error call with the safe
representation to avoid NPE in SendNotification.
Comment on lines
+102
to
117
| private String trimDescription(String jsonStr) { | ||
| JsonObject jo = (JsonObject) JsonParser.parseString(jsonStr); | ||
| JsonObject payloadObj = (JsonObject) jo.get("payload"); | ||
| if (payloadObj != null) { | ||
| JsonElement descObj = payloadObj.get("description"); | ||
| if (descObj != null && descObj.getAsJsonPrimitive().isString()) { | ||
| String description = descObj.getAsString(); | ||
| if (description.length() > maxDescriptionLength) { | ||
| LOGGER.warn("Truncating description length={}", description.length()); | ||
| payloadObj.addProperty("description", description.substring(0, maxDescriptionLength)); | ||
| return jo.toString(); | ||
| } | ||
| } | ||
| } | ||
| return jsonStr; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Gson JsonParser.parseString invalid JSON exception behavior
💡 Result:
Short answer
JsonParser.parseString is lenient by default and will accept some non‑standard input (e.g., bare values, single quotes, unquoted names in lenient mode), so it does not always throw on "invalid" JSON. It will throw com.google.gson.JsonSyntaxException / JsonParseException (or JsonIOException for I/O problems) when it detects malformed JSON that cannot be parsed (multiple top‑level elements or trailing data, badly formed structure, etc.). (javadoc.io)
Details / practical notes
- Javadoc: parseString declares it throws JsonSyntaxException/JsonParseException; the doc explicitly says parsing is performed in lenient mode and that exceptions are thrown for multiple top‑level elements or trailing data. (javadoc.io)
- Troubleshooting: older Gson defaults to lenient parsing (so malformed JSON may be accepted). Newer Gson releases add APIs to enforce strictness (GsonBuilder.setStrictness / JsonReader.setStrictness) to make parsing strict. If you need strict validation, use a strict JsonReader or configure strictness. (google.github.io)
- Community guidance: many recommend using JsonReader (in strict mode) to validate JSON rather than relying on JsonParser.parseString. (stackoverflow.com)
If you want, tell me your Gson version and example input and I’ll show whether parseString will accept it and how to force strict parsing.
Citations:
- 1: https://javadoc.io/static/com.google.code.gson/gson/2.12.1/com.google.gson/com/google/gson/JsonParser.html?utm_source=openai
- 2: https://javadoc.io/static/com.google.code.gson/gson/2.12.1/com.google.gson/com/google/gson/JsonParser.html?utm_source=openai
- 3: https://google.github.io/gson/Troubleshooting.html?utm_source=openai
- 4: https://stackoverflow.com/questions/43543232/gson-jsonparser-parse-cannot-recognize-invalid-json-format?utm_source=openai
🌐 Web query:
Gson JsonElement type checking methods isJsonObject isJsonPrimitive
💡 Result:
Short explanation and examples:
- JsonElement.isJsonObject() returns true if the element is a JsonObject; use it before calling getAsJsonObject() (otherwise getAsJsonObject() throws IllegalStateException). [1]
- JsonElement.isJsonPrimitive() returns true if the element is a JsonPrimitive (string, number, boolean); use it before getAsJsonPrimitive() or getAsString()/getAsInt() etc. [1][2]
Example:
JsonElement el = JsonParser.parseString(jsonString);
if (el.isJsonObject()) {
JsonObject obj = el.getAsJsonObject();
// use obj
} else if (el.isJsonPrimitive()) {
JsonPrimitive prim = el.getAsJsonPrimitive();
if (prim.isBoolean()) { boolean b = prim.getAsBoolean(); }
else { String s = prim.getAsString(); }
}References: Gson JsonElement Javadoc. [1] [2]
Sources:
[1] Gson JsonElement Javadoc (com.google.gson.JsonElement).
[2] Gson JsonPrimitive / JsonElement methods in Javadoc.
🏁 Script executed:
# Find the actual file and examine the context around trimDescription
find . -name "SendNotification.java" -type fRepository: PaladinCloud/CE
Length of output: 158
🏁 Script executed:
# Search for usage of trimDescription to understand how it's called
rg "trimDescription" --type java -B 2 -A 2Repository: PaladinCloud/CE
Length of output: 2469
🏁 Script executed:
# Check error handling around trimDescription calls - get more context
rg "trimDescription" --type java -B 5 -A 5Repository: PaladinCloud/CE
Length of output: 5274
🏁 Script executed:
# Look for the full method containing trimDescription calls to understand try-catch blocks
sed -n '100,200p' lambda-functions/notification-invoke-service/src/main/java/com/paladincloud/SendNotification.javaRepository: PaladinCloud/CE
Length of output: 870
Guard trimDescription against non‑JSON or non‑object payloads.
Unchecked casts and unsafe Gson method calls will throw exceptions on malformed JSON or non-object payloads. While both call sites wrap trimDescription() in try-catch, exceptions prevent the SNS message from being published—the notification silently fails to send.
Root causes:
(JsonObject) JsonParser.parseString(jsonStr)throwsJsonSyntaxExceptionon malformed JSON; throwsClassCastExceptionif the root is not an object.(JsonObject) jo.get("payload")throwsClassCastExceptionif the payload is not a JSON object.descObj.getAsJsonPrimitive().isString()throwsIllegalStateExceptionif the element is not a primitive.
Use defensive type checks (isJsonObject(), isJsonPrimitive()) and wrap parsing in try-catch to handle edge cases gracefully, returning the original string on failure.
Proposed fix (defensive parsing)
private String trimDescription(String jsonStr) {
- JsonObject jo = (JsonObject) JsonParser.parseString(jsonStr);
- JsonObject payloadObj = (JsonObject) jo.get("payload");
- if (payloadObj != null) {
- JsonElement descObj = payloadObj.get("description");
- if (descObj != null && descObj.getAsJsonPrimitive().isString()) {
- String description = descObj.getAsString();
- if (description.length() > maxDescriptionLength) {
- LOGGER.warn("Truncating description length={}", description.length());
- payloadObj.addProperty("description", description.substring(0, maxDescriptionLength));
- return jo.toString();
- }
- }
- }
- return jsonStr;
+ try {
+ JsonElement root = JsonParser.parseString(jsonStr);
+ if (!root.isJsonObject()) {
+ return jsonStr;
+ }
+ JsonObject jo = root.getAsJsonObject();
+ JsonElement payloadEl = jo.get("payload");
+ if (payloadEl != null && payloadEl.isJsonObject()) {
+ JsonObject payloadObj = payloadEl.getAsJsonObject();
+ JsonElement descObj = payloadObj.get("description");
+ if (descObj != null && descObj.isJsonPrimitive() && descObj.getAsJsonPrimitive().isString()) {
+ String description = descObj.getAsString();
+ if (description.length() > maxDescriptionLength) {
+ LOGGER.warn("Truncating description length={}", description.length());
+ payloadObj.addProperty("description", description.substring(0, maxDescriptionLength));
+ return jo.toString();
+ }
+ }
+ }
+ } catch (RuntimeException ex) {
+ LOGGER.warn("Skipping description trimming due to invalid JSON payload", ex);
+ }
+ return jsonStr;
}📝 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.
Suggested change
| private String trimDescription(String jsonStr) { | |
| JsonObject jo = (JsonObject) JsonParser.parseString(jsonStr); | |
| JsonObject payloadObj = (JsonObject) jo.get("payload"); | |
| if (payloadObj != null) { | |
| JsonElement descObj = payloadObj.get("description"); | |
| if (descObj != null && descObj.getAsJsonPrimitive().isString()) { | |
| String description = descObj.getAsString(); | |
| if (description.length() > maxDescriptionLength) { | |
| LOGGER.warn("Truncating description length={}", description.length()); | |
| payloadObj.addProperty("description", description.substring(0, maxDescriptionLength)); | |
| return jo.toString(); | |
| } | |
| } | |
| } | |
| return jsonStr; | |
| } | |
| private String trimDescription(String jsonStr) { | |
| try { | |
| JsonElement root = JsonParser.parseString(jsonStr); | |
| if (!root.isJsonObject()) { | |
| return jsonStr; | |
| } | |
| JsonObject jo = root.getAsJsonObject(); | |
| JsonElement payloadEl = jo.get("payload"); | |
| if (payloadEl != null && payloadEl.isJsonObject()) { | |
| JsonObject payloadObj = payloadEl.getAsJsonObject(); | |
| JsonElement descObj = payloadObj.get("description"); | |
| if (descObj != null && descObj.isJsonPrimitive() && descObj.getAsJsonPrimitive().isString()) { | |
| String description = descObj.getAsString(); | |
| if (description.length() > maxDescriptionLength) { | |
| LOGGER.warn("Truncating description length={}", description.length()); | |
| payloadObj.addProperty("description", description.substring(0, maxDescriptionLength)); | |
| return jo.toString(); | |
| } | |
| } | |
| } | |
| } catch (RuntimeException ex) { | |
| LOGGER.warn("Skipping description trimming due to invalid JSON payload", ex); | |
| } | |
| return jsonStr; | |
| } |
🤖 Prompt for AI Agents
In
`@lambda-functions/notification-invoke-service/src/main/java/com/paladincloud/SendNotification.java`
around lines 102 - 117, The trimDescription method currently casts and calls
Gson methods unsafely; wrap the initial parse in a try-catch (catch
JsonSyntaxException/RuntimeException) and return the original jsonStr on any
failure, and replace direct casts with safe checks: after parsing check
JsonElement.isJsonObject() before calling getAsJsonObject() (refer to JsonParser
and jo), check that jo.get("payload") exists and isJsonObject() before using
payloadObj, and verify descObj.isJsonPrimitive() and
getAsJsonPrimitive().isString() before calling getAsString(); only then truncate
using maxDescriptionLength and LOGGER.warn, otherwise return jsonStr unchanged.
arunpaladin
approved these changes
Jan 26, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
In addition to adding metrics to track performance, an issue with a far too long description has been fixed.
This description issue prevented the event from being sent - that description is now truncated to 1000 characters.
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.