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

Implement minutely probes #199

Merged
merged 5 commits into from
Mar 18, 2024
Merged

Implement minutely probes #199

merged 5 commits into from
Mar 18, 2024

Conversation

unflxw
Copy link
Contributor

@unflxw unflxw commented Mar 14, 2024

Implement minutely probes

Implement a minutely probes system, heavily borrowing from the one
in our Ruby integration.

Implement enable_minutely_probes config option

Implement a config option that globally enables or disables the
minutely probes system, in line with our other integrations.

When disabled by the config option, the minutely probes system can
still be manually enabled by calling appsignal.probes.start().

Ensure that calling this function multiple times will not result in
the probes running multiple times per minute.

Add a lock around registering, unregistering and executing probes
to prevent race conditions where e.g. a probe's unregistering takes
place halfway through its execution.

Rename _opentelemetry functions

These functions are already in the appsignal.opentelemetry method,
they do not need to have _opentelemetry in their name.

Test start and stop methods

Got in a bit of a pickle to make the thread initialization testable.
When client.start() was called, a thread was spawned, so I had to
implement a way to stop these threads from executing during other
tests (hence client.stop())

All other tests continue to call _run_probes directly because the
resulting behaviour is easier to reason about.

Fixes #129. See appsignal/test-setups#189 for a test setup.

Implement a minutely probes system, heavily borrowing from the one
in our Ruby integration.
@unflxw unflxw added the feature label Mar 14, 2024
@unflxw unflxw self-assigned this Mar 14, 2024
Comment on lines +9 to +19
from appsignal import probes, set_gauge

def new_carts(previous_carts=None):
current_carts = Cart.objects.all().count()

if previous_carts is not None:
set_gauge("new_carts", current_carts - previous_carts)

return current_carts

probes.register("new_carts", new_carts)
Copy link
Contributor Author

@unflxw unflxw Mar 14, 2024

Choose a reason for hiding this comment

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

@noccor I borrowed from your draft for this example :)

The probe taking an argument and returning a value is optional, though it avoids having to make queries that explicitly say "one minute ago". The following is also okay:

# takes no arguments
def new_carts():
  set_gauge("new_carts", count_new_carts_somehow())
  # returns nothing

probes.register("new_carts", new_carts)

Copy link

Choose a reason for hiding this comment

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

@unflxw - amazing! Thank you so much for getting this into action so quickly 🙇

@unflxw
Copy link
Contributor Author

unflxw commented Mar 15, 2024

Apologies to @jeffkreeftmeijer, who had already reviewed this, but I ended up significantly altering the implementation to make it thread-safe and test how the thread is started, so I'm asking for a re-review. 🙇

unflxw added 4 commits March 15, 2024 15:22
Implement a config option that globally enables or disables the
minutely probes system, in line with our other integrations.

When disabled by the config option, the minutely probes system can
still be manually enabled by calling `appsignal.probes.start()`.

Ensure that calling this function multiple times will not result in
the probes running multiple times per minute.

Add a lock around registering, unregistering and executing probes
to prevent race conditions where e.g. a probe's unregistering takes
place halfway through its execution.
These functions are already in the `appsignal.opentelemetry` method,
they do not need to have `_opentelemetry` in their name.
Got in a bit of a pickle to make the thread initialization testable.
When `client.start()` was called, a thread was spawned, so I had to
implement a way to stop these threads from executing during other
tests (hence `client.stop()`)

All other tests continue to call `_run_probes` directly because the
resulting behaviour is easier to reason about.
Update the diagnose tests submodule for `enable_minutely_probes` to
be expected for Python.
@unflxw unflxw force-pushed the implement-minutely-probes branch from f6216b7 to 41e9bc7 Compare March 15, 2024 14:22
@backlog-helper
Copy link


This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback

@unflxw unflxw merged commit 60967b6 into main Mar 18, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add minutely probes system
4 participants