Skip to content

added keyword matching functionality for real-browser-monitor#6097

Closed
bhelm wants to merge 16 commits intolouislam:masterfrom
bhelm:master
Closed

added keyword matching functionality for real-browser-monitor#6097
bhelm wants to merge 16 commits intolouislam:masterfrom
bhelm:master

Conversation

@bhelm
Copy link
Copy Markdown

@bhelm bhelm commented Sep 3, 2025

📋 Overview

  • What problem does this pull request address?

    • Real-browser monitors could only check HTTP status codes but couldn't verify that specific content was present on the page after JavaScript rendering. This made it impossible to detect when a web application loads successfully but displays error messages or missing content that would only be visible in the rendered page. This becomes a real deal breaker with modern web applications that only reference a single js file on the first request. Sometimes there are 100+ requests for additional js and content that are required to work in order for the page to display correctly, creating a lot potential for things to go wrong and the monitor to miss real problems.
  • What features or functionality does this pull request introduce or enhance?

    • Adds optional keyword checking functionality to real-browser monitors, allowing users to specify a keyword that must be present (or absent with invert mode) in the rendered page content. The implementation extracts all visible text from the fully-loaded page using Playwright's textContent() method and performs keyword matching. This enables monitoring of dynamic web applications that rely heavily on JavaScript for content rendering.
  • Resolves: Real Browser Allow keyword lookup #4068

🛠️ 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):

📄 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.

🦿 LLM Assistance Declaration

This implementation was developed with assistance from an AI assistant (Claude) for:

  • Code architecture and implementation patterns
  • Test case development and E2E test scenarios
  • Code quality improvements and linting fixes
  • Pull request documentation

I (the submitter) reviewed the code closely and tested it manually. Not so close attention was given to the provided tests. I also verified that the keyword input field appears on the other monitors ui.

🧪 Testing

test coverage added:

  • Backend unit tests: test/backend-test/test-real-browser-keyword.js - Tests keyword logic, invert functionality, edge cases
  • E2E integration tests: test/e2e/specs/monitor-form.spec.js - Tests UI form behavior and monitor creation
  • All existing tests pass: Verified backward compatibility with existing functionality

Test results:

  • ✅ 19/19 E2E tests passed
  • ✅ All backend unit tests passed
  • ✅ ESLint and StyleLint validation passed

🔧 Implementation Details

Files modified:

  • server/monitor-types/real-browser-monitor-type.js - Core keyword checking logic
  • src/pages/EditMonitor.vue - UI form updates
  • test/backend-test/test-real-browser-keyword.js - Unit tests (new)
  • test/e2e/specs/monitor-form.spec.js - E2E tests (enhanced)

Key features:

  • Text extraction using Playwright's page.textContent('body')
  • Text preprocessing (whitespace normalization)
  • Support for invert keyword functionality
  • Comprehensive error handling with informative messages
  • Full backward compatibility

📷 Screenshots or Visual Changes

UI Modifications: Added keyword and invert keyword fields to real-browser monitor form, matching the existing pattern used in HTTP keyword monitors.

image

Changes made:

  • Keyword input field (optional for real-browser monitors)
  • Invert keyword checkbox
  • Appropriate help text indicating the feature is optional
  • Consistent styling with existing monitor types

End result
image

https://vuetifyjs.com/en/getting-started/browser-support/ was choosen as the first request to this URL only references javascript and contains NO content.

Feature Description
Keyword Field Optional text input for specifying keywords to search for in rendered page content
Invert Keyword Checkbox to reverse logic (monitor fails when keyword IS found)
Form Validation Optional for real-browser (unlike required for HTTP keyword monitors)
Backward Compatibility Existing real-browser monitors work unchanged

This PR resolves a real world problem and extends the coverage of uptime kuma to web 2.0 applications with minimal changes.

Copy link
Copy Markdown
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.

I have not run the code but it seems solid.
A few corners need a bit more scrubbing.

Have you looked Into if we can reuse the text monitor condition logic (see the dns monitor) to reduce duplicate code?
I would prefer to have less code for maintenance reasons ^^

Comment thread server/monitor-types/real-browser-monitor-type.js
Comment thread src/pages/EditMonitor.vue Outdated
Comment thread test/backend-test/test-real-browser-keyword.js
Comment thread test/e2e/specs/monitor-form.spec.js Outdated
Comment thread server/monitor-types/real-browser-monitor-type.js Outdated
@bhelm
Copy link
Copy Markdown
Author

bhelm commented Sep 9, 2025

The PR now includes a backend test for the "real browser" monitor that runs an actual chromium browser.
I have extracted the test setup into an seperate file so it can be used for currently untested and future functionality.

To be able to test the backend code without requiring an actual http:// or https:// website, i have allowed data:// urls. Data URLs are self-contained and are not subject to the local file:// inclusion vulnerability.

Also while reviwing the code and debugging problems with the test never finishing, i noticed that the browser's page or context may never be explicitely removed if errors occur during its execution. I made it more defensive by using a finally clause and explicitely closing page and context in any case, which may or may not resolve #3788

Thank you for providing a review and feedback. I hope it is all resolved and good now.

@bhelm bhelm requested a review from CommanderStorm September 9, 2025 15:11
Comment thread test/backend-test/test-real-browser-keyword.js Outdated
Comment thread test/backend-test/test-real-browser-keyword.js Outdated
Comment thread test/backend-test/test-real-browser-keyword.js Outdated
Comment thread server/monitor-types/real-browser-monitor-type.js
Comment thread test/backend-test/test-real-browser-keyword.js Outdated
Comment thread test/backend-test/test-real-browser-keyword.js Outdated
@CommanderStorm CommanderStorm marked this pull request as draft October 2, 2025 22:47
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Oct 2, 2025
@Bud-Macaulay
Copy link
Copy Markdown

Just wondering the status of this PR?

I would like to migrate to v2 and drop my external playwright runner altogether, but would like to hold off until a feature like this is ready?

@bhelm
Copy link
Copy Markdown
Author

bhelm commented Oct 22, 2025

@CommanderStorm Any idea why the auto tests are now failing with "browserType.launch: Executable doesn't exist at /Users/runner/Library/Caches/ms-playwright/chromium-1084/chrome-mac/Chromium.app/Contents/MacOS/Chromium"?

It was working when i created the PR but its now failing to find the chrome. Im not sure if this problem only exists within the test environment or if it also affects production builds.

I think (based on my expirience of other projects) the problem happens because the testing container was built with playwright install and then the build installs a newer playwright version that expects a newer chrome "chromium-1084". but im not so deep in the build pipeline that i can really tell.

Maybe playwright was updated but the container not?

@CommanderStorm
Copy link
Copy Markdown
Collaborator

Our playwright version is fairly old. Maybe this is an issue. Not sure

@CommanderStorm
Copy link
Copy Markdown
Collaborator

Regarding this PR: have you considered if moving this monitor to be a condition based one (see the dns monitor for an example) with variables might be a better fit?

Currently this is still a lot of code

@bhelm
Copy link
Copy Markdown
Author

bhelm commented Oct 23, 2025

Regarding this PR: have you considered if moving this monitor to be a condition based one (see the dns monitor for an example) with variables might be a better fit?

Currently this is still a lot of code

As far as i can see, the dns condition based monitor allows to match the "record" against a string with a configurable "equals/not equals/contains/not contains/starts with/ends with..." operations. That makes some sense with shorter dns record strings, but i.e. starts with or equals is not that useful when matching website content.

transfering the condition matching logic to the real browser monitor would allow to match multiple strings (i.e. website contains "good" or "perfect" and not contains "bad". I can imagine a "regex" match type would also be useful.

I feel extending the matching logic to more conditions is a seperate feature. The current conditional logic (key word exists / inverted) is not that complicated and is not really contributing many LoC while covering the majority of use cases. Most code in the monitor is about handling the browser.

This PR adds an (for me game making) feature to verify that a Web 2.0 Page has actually loaded instead of just being a very complex monitor to check for a HTTP 200 on the first GET request. I would like to see it merged soon.

I have now added the playwright install to the auto-tests. No idea why it worked before without it, but now its working again. I have changed the command to only install chromium to avoid installing firefox, saving some disk space and time.

@bhelm
Copy link
Copy Markdown
Author

bhelm commented Oct 23, 2025

I actually plan to extend it later by keeping screenshots of error states so one can see what the problem was. Currently, the user knows the monitor failed but cant see if it was a blank page, an error page or a valid page without the correct content.

@CommanderStorm CommanderStorm added pr:needs review this PR needs a review by maintainers or other community members and removed pr:please address review comments this PR needs a bit more work to be mergable labels Oct 23, 2025
@CommanderStorm
Copy link
Copy Markdown
Collaborator

moving this monitor to be a condition based one (see the dns monitor for an example) with variables

Honestly, I don't want to maintain this functionality as-is.
It is a lot of code that would need to be changed in the future.

I would be open to maintaining a condition based one, but not adding the keyword monitor.

We are already a bit streched thin on maintenance (#6260), so adding "more random features which are not entirely where we want to go" is not sustainable.
I'd like to have this implemented, but I'd like it better integrated into the newer parts of the codebase, rather than the old parts of the codebase.

@CommanderStorm CommanderStorm added pr:please address review comments this PR needs a bit more work to be mergable and removed pr:needs review this PR needs a review by maintainers or other community members labels Jan 2, 2026
@CommanderStorm
Copy link
Copy Markdown
Collaborator

CommanderStorm commented Jan 9, 2026

@amddeus care to give constructive comments instead of an 👎🏻 or want to push this over the line which I set?

@CommanderStorm
Copy link
Copy Markdown
Collaborator

I think this PR is a bit stuck.
If someone wants to push this over the line, I would love this.

Lets close this for now.

@CSchulz
Copy link
Copy Markdown

CSchulz commented Feb 3, 2026

I think this PR is a bit stuck. If someone wants to push this over the line, I would love this.

Lets close this for now.

@CommanderStorm Please give me some hints, what you need or write me a mail, then I will look into it. I would like to have this feature as well.

@CommanderStorm
Copy link
Copy Markdown
Collaborator

What are you unclear about? I think the feedback above what I can maintain long term and what not should be relatively clear.

@bhelm
Copy link
Copy Markdown
Author

bhelm commented Feb 4, 2026

I really cant see what harm this PR does to code maintainability in contrast to the value it adds. the "real browser monitor" is defacto useless if it can't tell if a web 2.0 page was loaded successfully or not. If you want less code to maintain, remove the real browser monitor completely instead, as the normal HTTP check has the same production value at less resource costs.

This PR adds the Framework for real browser tests, this took WAY MORE work than the "check for string" feature itself and adds plenty of value for future features that may come. These are 10 hours of voluntary work you are throwing away, that someone else will have to do again in the future. The real browser implementation is currently 100% untested in the master branch ...

Adding the "check for string" feature was only the first stepping stone for more features. Currently, you can get the notification that there was a problem with the page, but you cant view a screenshot of that problem. This would be something i would have added later, but seeing this discussion makes me want to do my own fork instead, than to contribute to the main projekt. Its just a waste of time for me at that point. The time i have spent on this PR would have gotten me close to have a full featured web 2.0 monitoring platform now instead.

Also see #6097 (comment)

@CommanderStorm
Copy link
Copy Markdown
Collaborator

I understand the frustration, and I do appreciate the time and effort you put into this PR. This isn’t a judgment on the value of browser-based checks or the work you’ve done.

The issue is where this functionality is added. The current “real browser monitor” is more like legacy code that I already want to avoid extending the way I previously did further.
Splitting http - keyword - json - ... was a mistake.

Adding keyword checks there increases long-term maintenance in a part of the codebase I am trying to phase out, especially given our limited maintainer capacity (#6260).
Moving to share the code how we check will reduce one-off bugs and as such increase how I am able to maintain this.
If you want to help out with maintainer capacity on this part of the codebase, there is a path towards this.

This isn’t about throwing work away or dismissing the idea. I am open to a condition-based browser monitor that’s properly integrated into the newer architecture. What I can’t do is keep growing the old monitor with new features.

If you’d like to explore adapting this toward the condition-based system, I’m happy to discuss that. If not, I still genuinely appreciate the effort-even if the decision itself stands.
If you'd like to merge the tests without the keyword change, that is also something I would appreciate.
That something is maintanbale is a always subjective and I am going to stick with this call.

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

Development

Successfully merging this pull request may close these issues.

[Real Browser] Allow keyword lookup

4 participants