sentry-elixir icon indicating copy to clipboard operation
sentry-elixir copied to clipboard

Add option for generating custom cron monitor slugs

Open iautom8things opened this issue 1 year ago • 6 comments

We've recently adopted the Oban integration with Sentry but have noticed that some of our jobs do not play nice with the assumptions in the library. We have a pattern of reuse of cron workers where there's an argument that is the key differentiator. As one example, this might be a client_id.

We would like to be able to monitor these cron jobs separately.

I would be happy to add/improve more testing and/or documentation. I really just wanted to get this opened as a starting point for discussion.

šŸ™ Thank you for this consideration.

iautom8things avatar Sep 26 '24 16:09 iautom8things

Thanks for the PR. This sounds like a good addition to me. I approved the test run but unfortunately the tests didn't pass, any chance you could take a look and make them pass? šŸ˜„

solnic avatar Sep 30 '24 11:09 solnic

Thanks for the PR. This sounds like a good addition to me. I approved the test run but unfortunately the tests didn't pass, any chance you could take a look and make them pass? šŸ˜„

Edit: I started typing this response out but then had an aha moment about what the cause could be for my flaky tests. Those are fixed, but there's still a test that fails randomly on master. See below:


~That's odd.~ In truth, I was testing my tests in isolation with mix test --only custom_monitor_name_generator. For one, just to speed the testing up, but also it seems that the tests for the project are flaky, at least on master.

Here's a seed on my branch that my tests pass, but there's another test that fails:

 ✘ mz@mz-deca ī‚° ~/s/sentry-elixir ī‚° ī‚  task/allow-custom-cron-monitor-name-generation ā ✱ ī‚°
 [orbstack|default] āÆ mix test --seed 441419                                                   [10:00:16]
Wrote 37 files in 265.39 kb to: _build/test/lib/sentry/priv/sentry.map
...............................

  1) test with Plug sends two errors when a Plug process crashes if cowboy domain is not excluded (Sentry.LoggerHandlerTest)
     test/sentry/logger_handler_test.exs:99
     Assertion with == failed
     code:  assert second_event.original_exception == %RuntimeError{message: "Error"}
     left:  nil
     right: %RuntimeError{message: "Error"}
     stacktrace:
       test/sentry/logger_handler_test.exs:109: (test)

...............................................................................................................................................................................................................................................................
Finished in 2.1 seconds (0.6s async, 1.4s sync)
13 doctests, 274 tests, 1 failure

Randomized with seed 441419


On master the following test fails:

mix test --seed 30420

But the following test passes:

mix test --seed 310035


I just cleaned up the warnings about the unused variable. And I also believe I found the cause for my tests to fail, and fixed them.

But there still appears to be the same flaky test from above (test/sentry/logger_handler_test.exs:99) that fails about every 3rd attempt.

iautom8things avatar Sep 30 '24 15:09 iautom8things

I wont have time to review this this week as I’m on vacay, will review on Monday.

whatyouhide avatar Oct 01 '24 16:10 whatyouhide

On master the following test fails:

mix test --seed 30420

I can't reproduce this. I also left running tests for like over an hour in a loop and they didn't fail :/

solnic avatar Oct 08 '24 09:10 solnic

On master the following test fails: mix test --seed 30420

I can't reproduce this. I also left running tests for like over an hour in a loop and they didn't fail :/

This appears it may be related to the elixir/otp version? It errors for me on 1.16.2 / 26.2.5 but if I upgrade to 1.17.2 / 27.0.1 it doesn't fail.

Edit: I did a little more testing...

1.16.2 / 26.2.5.3 āŒ 1.16.3 / 26.2.5.3 āœ…

So, it looks like it's fixed with elixir 1.16.3. Maybe this isn't an issue that needs to be addressed.

iautom8things avatar Oct 08 '24 18:10 iautom8things

Thank you, @whatyouhide, for all of the suggestions! :bow:

iautom8things avatar Oct 10 '24 21:10 iautom8things