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 option for generating custom cron monitor slugs #803

Conversation

iautom8things
Copy link
Contributor

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.

@solnic
Copy link
Collaborator

solnic commented Sep 30, 2024

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? 😄

@iautom8things
Copy link
Contributor Author

iautom8things commented Sep 30, 2024

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 iautom8things requested a review from savhappy October 1, 2024 00:56
@whatyouhide
Copy link
Collaborator

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

@solnic
Copy link
Collaborator

solnic commented Oct 8, 2024

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 :/

@iautom8things
Copy link
Contributor Author

iautom8things commented Oct 8, 2024

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.

Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

Nice job! Left some comments but this is looking like a great start 🙃

lib/sentry/config.ex Outdated Show resolved Hide resolved
lib/sentry/integrations/oban/cron.ex Outdated Show resolved Hide resolved
test/sentry/integrations/oban/cron_test.exs Outdated Show resolved Hide resolved
lib/sentry/config.ex Outdated Show resolved Hide resolved
lib/sentry/config.ex Outdated Show resolved Hide resolved
lib/sentry/config.ex Outdated Show resolved Hide resolved
lib/sentry/integrations/oban/cron.ex Outdated Show resolved Hide resolved
test/sentry/integrations/oban/cron_test.exs Outdated Show resolved Hide resolved
@iautom8things
Copy link
Contributor Author

Thank you, @whatyouhide, for all of the suggestions! 🙇

Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

@savhappy review and merge when you get the time plz :)

@whatyouhide whatyouhide changed the title add option for generating custom cron monitor slugs Add option for generating custom cron monitor slugs Oct 11, 2024
@whatyouhide whatyouhide merged commit c529713 into getsentry:master Oct 21, 2024
4 checks passed
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.

4 participants