fix: Don't send events with null to Segment
#32600
Conversation
WalkthroughWalkthroughThis update refines error handling in the Changes
Possibly related issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SegmentConfig.java
Show resolved
Hide resolved
| final String stackTrace = | ||
| ObjectUtils.defaultIfNull(error == null ? null : ExceptionUtils.getStackTrace(error), ""); | ||
|
|
||
| if (logData.getMessage() == null) { |
There was a problem hiding this comment.
Nit, could you please prepend a common qualifier to these logs so it's easy to check for any kind of failure in logs?
There was a problem hiding this comment.
I initially did that, but noticed that this log object is for this class, so the log line should show SegmentConfig in it, right? Doesn't it? 🤔
There was a problem hiding this comment.
Nah I don't think we print that info in our logs, we've explicitly excluded it in our logging pattern.
There was a problem hiding this comment.
I've approved the PR anyway, this can be an addition, since we might want to fast-track cherry-picking this to master.
There was a problem hiding this comment.
Hm., I could've sworn we included the category into the log format. 🤷
Perhaps we can add that in? 🤔
Recently, we made two fixes to `SegmentConfig` to fix NPEs that were preventing some events from being sent to Segment. 1. #32498 2. #32351 But this ended up sending _too much_ to Segment now. That, and clearly we weren't missing anything by not sending those events with `null` in them. So we're bringing back that protection of NPEs. The protection of not sending value-less events to Segment. But in a more, educated way. We're adding `null` checks, and not sending anything when we see a `null`. Considering that context, a more accurate diff to review would be between the [`SegmentConfig` before those two PRs](https://github.com/appsmithorg/appsmith/blob/d6e74bf0122b0e5a91e175c3d2a70fef9fd68bb1/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SegmentConfig.java), and the `SegmentConfig` in this PR. This diff can be seen here. (cherry picked from commit c831644)
Recently, we made two fixes to
SegmentConfigto fix NPEs that were preventing some events from being sent to Segment.But this ended up sending too much to Segment now. That, and clearly we weren't missing anything by not sending those events with
nullin them.So we're bringing back that protection of NPEs. The protection of not sending value-less events to Segment. But in a more, educated way.
We're adding
nullchecks, and not sending anything when we see anull.Considering that context, a more accurate diff to review would be between the
SegmentConfigbefore those two PRs, and theSegmentConfigin this PR. This diff can be seen here.