-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add template context fields for Elastic integration #6504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Claude finished @brant-livefront's task —— View job Code Review Complete ✓Tasks
Summary of ChangesThis PR exposes 9 additional ID fields from New ID Fields:
New Formatted Property:
Critical IssuesAccording to the project's CLAUDE.md guidelines: "ALWAYS add unit tests (with mocking) for any new feature development" The new Recommended Test CasesAdd tests to [Theory, BitAutoData]
public void DateIso8601_ReturnsIso8601FormattedDate(EventMessage eventMessage)
{
var testDate = new DateTime(2025, 10, 27, 13, 30, 0, DateTimeKind.Utc);
eventMessage.Date = testDate;
var sut = new IntegrationTemplateContext(eventMessage);
var result = sut.DateIso8601;
Assert.Equal("2025-10-27T13:30:00.0000000Z", result);
// Verify it's valid ISO 8601
Assert.True(DateTime.TryParse(result, out _));
}
[Theory, BitAutoData]
public void DateIso8601_HandlesLocalTime(EventMessage eventMessage)
{
var localDate = new DateTime(2025, 10, 27, 13, 30, 0, DateTimeKind.Local);
eventMessage.Date = localDate;
var sut = new IntegrationTemplateContext(eventMessage);
var result = sut.DateIso8601;
// Verify format includes timezone offset
Assert.Matches(@"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}", result);
}Suggested Improvements🎨 Potential Timezone Consideration The ContextIf public string DateIso8601 => Date.ToUniversalTime().ToString("o");However, if 📝 Minor: PR Title Typo The PR title contains a typo: "Elasic" should be "Elastic" Good Practices Observed👍 Complete Field Coverage - All relevant ID fields from Action Items
Security & Performance AssessmentSecurity: ✅ No security concerns. The changes expose existing data fields without introducing new vulnerabilities. All exposed fields are GUIDs or formatted dates - no sensitive data leakage. Performance: ✅ Minimal impact. The VerdictThe implementation is clean and straightforward, but unit tests are required before merging per project guidelines. Once tests are added for |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6504 +/- ##
==========================================
- Coverage 52.02% 52.02% -0.01%
==========================================
Files 1878 1878
Lines 82706 82716 +10
Branches 7322 7322
==========================================
Hits 43030 43030
- Misses 38018 38028 +10
Partials 1658 1658 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
withinfocus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some good Claude callouts for improvements but works for me at the moment.

📔 Objective
While building a POC integration to Elastic, I discovered that they want a specific timestamp date format (ISO 8601). In addition, I noticed that there were a very fields on our
EventMessageobject that I hadn't exposed on theIntegrationTemplateContextobject. This PR adds these fields to the template context to allow them to be easily picked up in templates and specifically used as part of the Elastic integration.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes