added keyword matching functionality for real-browser-monitor#6097
added keyword matching functionality for real-browser-monitor#6097bhelm wants to merge 16 commits intolouislam:masterfrom
Conversation
CommanderStorm
left a comment
There was a problem hiding this comment.
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 ^^
Co-authored-by: Frank Elsinga <frank@elsinga.de>
|
The PR now includes a backend test for the "real browser" monitor that runs an actual chromium browser. 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. |
|
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? |
|
@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? |
|
Our playwright version is fairly old. Maybe this is an issue. Not sure |
|
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. |
|
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. |
Honestly, I don't want to maintain this functionality as-is. 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. |
|
@amddeus care to give constructive comments instead of an 👎🏻 or want to push this over the line which I set? |
|
I think this PR is a bit stuck. 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. |
|
What are you unclear about? I think the feedback above what I can maintain long term and what not should be relatively clear. |
|
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) |
|
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. 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). 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. |
📋 Overview
What problem does this pull request address?
What features or functionality does this pull request introduce or enhance?
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
📄 Checklist
🦿 LLM Assistance Declaration
This implementation was developed with assistance from an AI assistant (Claude) for:
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:
test/backend-test/test-real-browser-keyword.js- Tests keyword logic, invert functionality, edge casestest/e2e/specs/monitor-form.spec.js- Tests UI form behavior and monitor creationTest results:
🔧 Implementation Details
Files modified:
server/monitor-types/real-browser-monitor-type.js- Core keyword checking logicsrc/pages/EditMonitor.vue- UI form updatestest/backend-test/test-real-browser-keyword.js- Unit tests (new)test/e2e/specs/monitor-form.spec.js- E2E tests (enhanced)Key features:
page.textContent('body')📷 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.
Changes made:
End result

https://vuetifyjs.com/en/getting-started/browser-support/ was choosen as the first request to this URL only references javascript and contains NO content.
This PR resolves a real world problem and extends the coverage of uptime kuma to web 2.0 applications with minimal changes.