Add preconfigutred Cron Services #719

will2877 · 2023-05-09T13:37:08Z

This PR adds a pre-configured service to the Symfony service container that creates CronJobs and CheckIns that are configured using the bundle configuration.

To use it simply inject the Sentry\SentryBundle\Cron\CronJobFactoryInterface and create new CronJobs:

use Sentry\SentryBundle\Cron\CronJobFactoryInterface;

class ExampleCommand extends Command
{
    public function __construct(
        private readonly CronJobFactoryInterface $cronJobFactory
    ) {
    }

    public function execute(array $options, array $arguments, ?SymfonyStyle $io): int
    {
        $cronJob = $this->cronJobFactory->getCronjob('demo');
        $startCheckIn = $cronJob->inProgress();

        try {
            // Do some Stuff.....
            $cronJob->ok($startCheckIn);
        } catch (\Exception $exception) {
            // ... Log the error ...
            $cronJob->error($startCheckIn);
        }
        return 0;
    }
}

This PR also required an adjustment of the flex recipe. As the bundle is only loaded in production, we will need to manually register the service int dev:

when@prod:
# ... same as before ...
when@dev:
    services:
        Sentry\SentryBundle\Cron\CronJobFactoryInterface:
            class: Sentry\SentryBundle\Cron\CronJobFactory
            arguments:
                - 'dev'
                - ~

Jean85 · 2023-05-09T13:45:42Z

This PR also required an adjustment of the flex recipe. As the bundle is only loaded in production, we will need to manually register the service int dev:

And this is one of the reasons why I was against enabling it only in prod: symfony/recipes-contrib#1080 (review)

Unfortunately they went ahead with that anyway: symfony/recipes-contrib#1420

will2877 · 2023-05-11T06:40:04Z

The alternative would be to adjust the documentation and ask the user to create the service.

Is it planed to create a new release in the foreseeable future?
I would like to start using this in a few projects.

cleptric · 2023-05-15T18:04:09Z

Thanks for the contribution. I'll leave this open for the moment, as I'm unsure if I want to move in this direction.

In any case, getsentry/sentry-php#1511 needs to land first.

cleptric · 2023-07-25T18:16:12Z

@will2877 Upserts for Crons were released. Besides the wording, I would stick to CheckIn, I'm curious if Symfony also offers something similar to Laravel's scheduled taks, which would allow us a more automatic integration.

will2877 · 2023-07-25T18:35:59Z

@cleptric
Symfony introduced the Scheduler Component with Release 6.3 a few months ago.

Another approach would be to allow the crons to be configured in the bundle configuration, similar to the Scoped HttpClients.

# config/packages/sentry.yaml
sentry:
    monitors:
        # Cron Job
        sample.cronjob:
            slug: 'cronjob-demo'
            type: 'crontab'
            schedule: '*/5 * * * *'
            checkinMargin: 5,
            maxRuntime: 30,
            timezone: 'UTC',
        # Interval
        sample.interval:
            slug: 'interval-demo'
            type: 'interval' # or interval
            schedule:
                value: 30
                unit: 'minute' # or hour, day, etc.
            checkinMargin: 5
            maxRuntime: 30
            timezone: 'UTC'

I still need need to check in detail if and how this is possible.

getsentry/sentry-php#1511

cleptric

Codecov points out quite some areas that are not covered by tests. Could you cover those?

cleptric · 2023-07-26T11:14:35Z

src/Monitor/Monitor.php

This class seems to create a CheckIn, not a Monitor.

cleptric · 2023-07-26T11:16:46Z

src/DependencyInjection/SentryExtension.php

@@ -154,6 +148,23 @@ private function registerConfiguration(ContainerBuilder $container, array $confi
            ->setDefinition('sentry.client', new Definition(Client::class))
            ->setPublic(false)
            ->setFactory([$clientBuilderDefinition, 'getClient']);
+
+        // Setup Monitors.
+        $environment = '';


An event should always have an environment set and defaults to production. See https://github.com/getsentry/sentry-php/blob/master/src/Event.php#L19

cleptric · 2023-07-26T11:17:18Z

src/Monitor/MonitorFactory.php

Same here, this is about a CheckIn.

cleptric · 2023-07-26T11:17:37Z

src/Monitor/MonitorFactoryInterface.php

Same naming issuse

cleptric · 2023-07-26T11:17:40Z

src/Monitor/MonitorInterface.php

Same naming issuse

cleptric · 2023-07-26T11:17:58Z

tests/Cron/CronJobFactoryTest.php

You likely want to rename the class and file.

ste93cry · 2023-07-26T12:18:06Z

In all honesty, I’m having an hard time understanding what we’re trying to achieve here. The factory that is just a proxy to create a Monitor class seems an overkill to start with 😒

will2877 · 2023-07-26T12:47:53Z

The factory is required to inject the environment and release from the configuration file:

# config/packages/sentry.yaml
when@prod:
    sentry:
        dsn: '%env(SENTRY_DSN)%'
        options:
            environment: 'production' # <--
            release: '1.1.8' # <--

Debatable is if the FactoryInterface as well as the MonitorInterface are necessary or not.

ste93cry · 2023-07-26T13:19:00Z

Why not simply add a captureMonitor() method on the HubInterface and/or a global function to the base SDK? While reading the documentation of the JS SDK, it seems that’s the way crons have been implemented there. No factories required, a simple and well integrated solution in the base SDK rather than a complex one in this bundle. Or am I missing something that that solution wouldn’t support?

cleptric · 2023-07-26T13:38:14Z

We already have all this functionality in the base SDK, see https://docs.sentry.io/platforms/php/crons/

I would assume some more Symfony-like integration would make sense, but I'm not sure how this should look like.

ste93cry · 2023-07-26T14:05:24Z

We already have all this functionality in the base SDK

I can't find a captureCheckIn() method, the documentation talks about the generic captureEvent() method at best. Moreover, what's the benefit of having pre-configured cron services? I've never used this functionality before so I may not have all the context, but I fail to see the benefits of this tight "integration" with the framework and which use cases it solves. I'm also unsure that the config should be extended for the monitors: what's the benefit of this over just calling a method and passing the config directly to it?

// Example ported from the JS documentation
$checkInId = \Sentry\captureCheckIn('<monitor-slug>', \Sentry\CheckInStatus::inProgress());

// Execute your scheduled task here...

// Notify Sentry your job has completed successfully:
\Sentry\captureCheckIn('<monitor-slug>', \Sentry\CheckInStatus::ok(), $checkInId);

will2877 · 2023-07-26T14:07:22Z

Seeing as there is only scarce documentation on the scheduler component, and not everyone will adopt this immediately i would refrain from coupling this.

I would stick to the solution suggested here without the interfaces.
The new service can be called using dependency injection (Autowiring) and used to create the monitor.
Those services that are built on top of the scheduler component will probably also be configured using Autowiring.

At a later point the pre-configured Monitor may also be Autowired in a separate PR as suggested above:

Another approach would be to allow the crons to be configured in the bundle configuration, similar to the Scoped HttpClients.

# config/packages/sentry.yaml
sentry:
    monitors:
        # Cron Job
        sample.cronjob:
            slug: 'cronjob-demo'
            type: 'crontab'
            schedule: '*/5 * * * *'
            checkinMargin: 5,
            maxRuntime: 30,
            timezone: 'UTC',
        # Interval
        sample.interval:
            slug: 'interval-demo'
            type: 'interval' # or interval
            schedule:
                value: 30
                unit: 'minute' # or hour, day, etc.
            checkinMargin: 5
            maxRuntime: 30
            timezone: 'UTC'

I still need need to check in detail if and how this is possible.

will2877 · 2023-07-26T14:14:51Z

This dose not make use of the MonitorConfig introduced in #getsentry/sentry-php#1511 but i am not completely opposed to this approach.

// Example ported from the JS documentation
$checkInId = \Sentry\captureCheckIn('<monitor-slug>', \Sentry\CheckInStatus::inProgress());

// Execute your scheduled task here...

// Notify Sentry your job has completed successfully:
\Sentry\captureCheckIn('<monitor-slug>', \Sentry\CheckInStatus::ok(), $checkInId);

ste93cry · 2023-07-26T14:22:20Z

Yes, I just copied the first example I found in the doc. Actually, the definition of the captureCheckIn() method in the JS SDK is the following:

export function captureCheckIn(checkIn: CheckIn, upsertMonitorConfig?: MonitorConfig): string

Personally, I stand by my opinion that I don't see any new benefit in a solution like the one suggested in this PR as this method seems to provide already everything a user need. Maybe we could argue that defining the monitors in the config centralizes everything in one place, but you will have anyway to "capture the checkin" and its status in the code, so it's quite fragile as an answer. At that point, I hardly imagine why I wouldn't want to create the monitor where I need to use it, which also eases looking for occurrences in the code if I need to.

will2877 · 2023-07-26T14:39:18Z

If I am correct, this need to be added to the HubInterface:

$hub = SentrySdk::getCurrentHub();
$monitorConfig = new \Sentry\MonitorConfig(
    \Sentry\MonitorSchedule::crontab('*/5 * * * *'),
    5,
    30,
    'UTC'
);
$checkIn = $hub->captureCheckin('<monitor-slug>', $monitorConfig, \Sentry\CheckInStatus::inProgress());
/**
 * Some important task....
 */
$checkIn = $hub->captureCheckin('<monitor-slug>', $monitorConfig, \Sentry\CheckInStatus::ok(), $checkIn);

will2877 · 2023-08-02T17:12:40Z

Closing in favour of getsentry/sentry-php#1573

will2877 added 5 commits May 9, 2023 07:45

Add interfaces and service implenetations.

d9a50ee

Add test.

86eaf7c

Add missing return value to test.

da7d961

Register Service.

1c30d26

Make release optional.

5ed136f

cleptric added Type: Feature Status: In Progress labels May 15, 2023

hubertdeng123 removed the Status: In Progress label Jul 25, 2023

cleptric added the Waiting for: Community label Jul 25, 2023

will2877 and others added 2 commits July 25, 2023 21:29

Adjust wording and add Cron Monitor upsert

32c1f61

getsentry/sentry-php#1511

Merge branch 'getsentry:master' into add_cron_service

f168b33

cleptric reviewed Jul 26, 2023

View reviewed changes

will2877 mentioned this pull request Jul 26, 2023

Add captureCheckIn() getsentry/sentry-php#1573

Merged

will2877 closed this Aug 2, 2023

cleptric mentioned this pull request Aug 29, 2023

Add an easy cron integration #757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preconfigutred Cron Services #719

Add preconfigutred Cron Services #719

will2877 commented May 9, 2023

Jean85 commented May 9, 2023

will2877 commented May 11, 2023

cleptric commented May 15, 2023

cleptric commented Jul 25, 2023

will2877 commented Jul 25, 2023 •

edited

Loading

cleptric left a comment

cleptric Jul 26, 2023

cleptric Jul 26, 2023

cleptric Jul 26, 2023

cleptric Jul 26, 2023

cleptric Jul 26, 2023

cleptric Jul 26, 2023

ste93cry commented Jul 26, 2023

will2877 commented Jul 26, 2023

ste93cry commented Jul 26, 2023 •

edited

Loading

cleptric commented Jul 26, 2023

ste93cry commented Jul 26, 2023

will2877 commented Jul 26, 2023

will2877 commented Jul 26, 2023

ste93cry commented Jul 26, 2023 •

edited

Loading

will2877 commented Jul 26, 2023

will2877 commented Aug 2, 2023

Add preconfigutred Cron Services #719

Add preconfigutred Cron Services #719

Conversation

will2877 commented May 9, 2023

Jean85 commented May 9, 2023

will2877 commented May 11, 2023

cleptric commented May 15, 2023

cleptric commented Jul 25, 2023

will2877 commented Jul 25, 2023 • edited Loading

cleptric left a comment

Choose a reason for hiding this comment

cleptric Jul 26, 2023

Choose a reason for hiding this comment

cleptric Jul 26, 2023

Choose a reason for hiding this comment

cleptric Jul 26, 2023

Choose a reason for hiding this comment

cleptric Jul 26, 2023

Choose a reason for hiding this comment

cleptric Jul 26, 2023

Choose a reason for hiding this comment

cleptric Jul 26, 2023

Choose a reason for hiding this comment

ste93cry commented Jul 26, 2023

will2877 commented Jul 26, 2023

ste93cry commented Jul 26, 2023 • edited Loading

cleptric commented Jul 26, 2023

ste93cry commented Jul 26, 2023

will2877 commented Jul 26, 2023

will2877 commented Jul 26, 2023

ste93cry commented Jul 26, 2023 • edited Loading

will2877 commented Jul 26, 2023

will2877 commented Aug 2, 2023

will2877 commented Jul 25, 2023 •

edited

Loading

ste93cry commented Jul 26, 2023 •

edited

Loading

ste93cry commented Jul 26, 2023 •

edited

Loading