Skip to content

Conversation

@kevgoehl
Copy link

  • Add response column to heartbeat table to store HTTP response data
  • Add monitor configuration options: saveResponse (default: false) and responseMaxLength (default: 10240 bytes)
  • Response max length of 0 allows unlimited storage without truncation
  • Response data accessible in notification templates via heartbeatJSON.response
  • Add UI controls in monitor edit page with translations (EN, DE-DE, DE-CH)
  • Only saves response data when explicitly enabled to minimize database impact

Resolves #6164

❗ Important Announcements

Click here for more details:

⚠️ Please Note: We do not accept all types of pull requests, and we want to ensure we don’t waste your time. Before submitting, make sure you have read our pull request guidelines: Pull Request Rules

🚫 Please Avoid Unnecessary Pinging of Maintainers

We kindly ask you to refrain from pinging maintainers unless absolutely necessary. Pings are for critical/urgent pull requests that require immediate attention.

📋 Overview

  • What problem does this pull request address?
    • Please provide a detailed explanation here.
  • What features or functionality does this pull request introduce or enhance?
    • Please provide a detailed explanation here.
  • Relates to #issue-number
  • Resolves #issue-number

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)
  • 📄 Documentation Update (modification of existing documentation)
  • 📄 Documentation Update Required (the change requires updates to related documentation)
  • 🔧 Other (please specify):
    • Provide additional details here.

📄 Checklist

  • 🔍 My code adheres to the style guidelines of this project.
  • 🦿 I have indicated where (if any) I used an LLM for the contributions
  • ✅ I ran ESLint and other code linters for modified files.
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🤖 My code needed automated testing. I have added them (this is an optional task).
  • 📄 Documentation updates are included (if applicable).
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • 📚 I have read and understood the Pull Request guidelines.

📷 Screenshots or Visual Changes

  • UI Modifications: Highlight any changes made to the user interface.
    When the save Response is activated:
image

When the save response is deactivated:
image

Event Before After
UP Before After
DOWN Before Always "null"

If you need more information or context, feel free to contact me.

…lam#6164)

- Add `response` column to heartbeat table to store HTTP response data
- Add monitor configuration options: `saveResponse` (default: false) and `responseMaxLength` (default: 10240 bytes)
- Response max length of 0 allows unlimited storage without truncation
- Response data accessible in notification templates via `heartbeatJSON.response`
- Add UI controls in monitor edit page with translations (EN, DE-DE, DE-CH)
- Only saves response data when explicitly enabled to minimize database impact

Resolves louislam#6164
@kevgoehl kevgoehl force-pushed the feat/6164-save-response-for-notifications branch from b2551d2 to d44ee3a Compare October 17, 2025 05:39
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks reasonable.

I am not sure if the max chars should be a constant setting. 🤔

I don't quite remember if adding something just to the heartbeat table works. => does the UptimeCalculator leave entries which have not yet been notifed for or how do these two systems interact?

Adding the stored values to the fronted (where it exists) would be going above and beyond. Not sure if you want to do this or if we should open a follow up issue for this

- Use i18n-t component to display template variable as formatted code
- Refactor responseMaxLength using nullish coalescing operator
- Update translation en.json for clarity
@kevgoehl
Copy link
Author

Overall, looks reasonable.

I am not sure if the max chars should be a constant setting. 🤔

I don't quite remember if adding something just to the heartbeat table works. => does the UptimeCalculator leave entries which have not yet been notifed for or how do these two systems interact?

Adding the stored values to the fronted (where it exists) would be going above and beyond. Not sure if you want to do this or if we should open a follow up issue for this

Hi @CommanderStorm,
thank you for the CR.

  1. Question: max chars as constant:
  • I made it per-monitor configurable since different APIs have vastly different response sizes.
  1. Question: UptimeCalculator
  • When i understand the UptimeCalculator correctly, it only receives status/ping for aggregate stats and doesn't interact with the response field. The heartbeat table addition is sufficient since notifications receive the full heartbeat bean via toJSON().
  1. Question: frontend display
  • I focused on the notification use case for this PR. Happy to open a follow-up issue for adding response display to the Details page/dashboard.

@kevgoehl kevgoehl force-pushed the feat/6164-save-response-for-notifications branch from 912fdca to 7b0bb77 Compare October 29, 2025 07:42

<!-- HTTP / Keyword only -->
<template v-if="monitor.type === 'http' || monitor.type === 'keyword' || monitor.type === 'json-query' || monitor.type === 'grpc-keyword' ">
<template v-if="monitor.type === 'http' || monitor.type === 'keyword' || monitor.type === 'json-query'">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that the change to grpc-keyword is resolved/removed, we can merged.

GRPC does currently not log, but should have redirects.

@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Dec 23, 2025
@CommanderStorm CommanderStorm added this to the 2.2.0 milestone Dec 23, 2025
@CommanderStorm
Copy link
Collaborator

(put it in 2.2 instead of 2.1 due to the grpc issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs:resolve-merge-conflict pr:please address review comments this PR needs a bit more work to be mergable

Projects

None yet

2 participants