dynatrace-extensions / dt-extensions-python-sdk

Dynatrace Python Extensions SDK
https://dynatrace-extensions.github.io/dt-extensions-python-sdk/
MIT License
8 stars 2 forks source link

schedule() with a timedelta of '0' results in continuous execution #61

Closed JosephHobbs closed 1 month ago

JosephHobbs commented 1 month ago

Describe the bug

During extension initialization you can schedule additional methods to execute by providing a timedelta object. If you create a timedelta object that equates to zero [ex. timedelta(minutes=0)] the method will run continuously with no delay.

To Reproduce Steps to reproduce the behavior:

Within extension, schedule a method to execute using a timedelta that resolves to '0'...

self.schedule(self.my_method, timedelta(minutes=0))

Expected behavior

If set to 0 I would expect one of two things to occur...

Personally I'd vote for the 'do not schedule' option, mainly so it can be used as a way to disable the execution of that particular thread at runtime (i.e. configurable interval). I can wrap my schedule in a 0 check, but it would be nice if schedule did it for me.

I'm not sure if there is a use case for running continuously? If so, maybe use something else (like null?) to indicate that is the desired action (versus 0)?

Desktop (please complete the following information):

dlopes7 commented 1 month ago

There is currently no API to stop something that was already schedule, and I don't think adding an special case for a 0 timedelta would be a good API for that

I do agree that there should be an API to stop a method from being scheduled

Anyways, we can add an exception if the timedelta is less than 1 second.

JosephHobbs commented 1 month ago

Just to clarify (for my understanding)...

Assume I include a config option in my extension that includes the interval to execute. I set it to 10m and it's running. If I go into Dynatrace and change that configuration to 5m and save. Does the EEC not stop/start the extension to account for the config change?

dlopes7 commented 1 month ago

The EEC stops the entire python process and starts over with the updated monitoring configuration, so yes that does get updated.

What I mean is that from the code you can't change or stop a running scheduled method.

JosephHobbs commented 1 month ago

Yup, I get that. The use case I'm after is more config-based interval control. So it's all in the settings and would really only matter at initialization. So I'm not touching any existing scheduled threads/etc. running.

Appreciate the conversation/insight!

dlopes7 commented 1 month ago

I see, this is more about trusting user input.

Just a note, when giving users option to provide input to the extension we must check that the data we are working with is valid

And you can even restrict that on monitoring configuration time, example:

"frequency": {
          "displayName": "Frequency",
          "description": "How often should we collect metrics in minutes",
          "type": "integer",
          "nullable": false,
          "default": 1,
          "metadata": {
            "suffix": "minutes"
          },
          "constraints": [
            {
              "type": "RANGE",
              "minimum": 1,
              "maximum": 60
            }
          ]
        }
JosephHobbs commented 1 month ago

Most definitely. I would include sensible constraints to ensure the values entered aren't too crazy. Thanks!