Skip to content
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

Add script probe type #1053

Closed
wants to merge 19 commits into from
Closed

Add script probe type #1053

wants to merge 19 commits into from

Conversation

jgillick
Copy link
Contributor

@jgillick jgillick commented May 31, 2023

Monika Pull Request (PR)

What feature/issue does this PR add

  • Feature: Adds a script probe type that will execute a local script for slightly more complex probes. The probe status will be the return value of the script (0 == "success").
  • Fix: The prometheus feature was breaking for non-request probes. (cannot destructure request in collector.ts)

How did you implement / how did you fix it

  1. Using the postgres probe as an example, I created a new script probe.
  2. Wrote unit tests for the probe.
  3. Added documentation.

How to test

monika.yml

probes:
  - id: '1'
    name: 'Script tests'
    script:
      - cmd: 'echo Hello World'
      - cmd: exit 123
      - cmd: sh probe.sh

probe.sh

#!/bin/sh
echo "Probing things..."
exit 0

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (e42bb39) 63.60% compared to head (faa09d3) 61.98%.

❗ Current head faa09d3 differs from pull request most recent head 4559daf. Consider uploading reports for the commit 4559daf to get more accurate results

Files Patch % Lines
src/components/probe/prober/script/index.ts 55.17% 13 Missing ⚠️
src/components/probe/prober/script/request.ts 73.07% 5 Missing and 2 partials ⚠️
src/components/probe/prober/factory.ts 33.33% 2 Missing ⚠️
src/plugins/metrics/prometheus/collector.ts 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1053      +/-   ##
==========================================
- Coverage   63.60%   61.98%   -1.63%     
==========================================
  Files         108      109       +1     
  Lines        3171     3222      +51     
  Branches      534      549      +15     
==========================================
- Hits         2017     1997      -20     
- Misses        981     1036      +55     
- Partials      173      189      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nicnocquee
Copy link
Member

Hello @jgillick sorry for responding late for this PR, we're still discussing and trying this internally. I tried this PR but I have a question:

When a probe has multiple cmds, say 3 commands, and the second command's exit is not 0, any reasons why the third command is still executed? The behaviour of monika when a HTTP probe has multiple requests, the requests are chained, and if one fails, the subsequent requests won't be executed.

@jgillick
Copy link
Contributor Author

Hi @nicnocquee. That's a great point. I can't see any reason they would continue to execute if one of them fails. I can update the PR sometime in the next week.

@jgillick
Copy link
Contributor Author

@nicnocquee I've updated the PR to stop executing scripts in a probe when one fails.

Copy link
Contributor

@sapiderman sapiderman left a comment

Choose a reason for hiding this comment

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

It's a great idea, and looks ok.

  1. Will need a new PR for the jsonschema
  2. The logs and notification maybe needs improvement later (see below).

image

@jgillick
Copy link
Contributor Author

jgillick commented Nov 28, 2023

@sapiderman I have updated the PR with the jsonschema updates and synced upstream.

@nicnocquee
Copy link
Member

Hey @jgillick apologies for the delayed response. We've been swamped with fixing implementations and integrating Monika with our SaaS. Unfortunately, we're closing this PR because we Monika is designed as a monitoring tool, and executing arbitrary scripts falls outside of its intended use. Thanks for your contribution, though. If the script execution feature is crucial for you, feel free to fork the project.

@nicnocquee nicnocquee closed this Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants