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

SchedulerFactoryBeanCustomizer now runs first so user customization is not overridden #3095

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Dec 13, 2023

📜 Description

We now set @Order(Ordered.HIGHEST_PRECEDENCE) on our SchedulerFactoryBeanCustomizer bean so it is called before any user SchedulerFactoryBeanCustomizer.

Due to limited API we cannot simply read all global job listeners or add ours to the list. All we can do is "set" a list containing our own thereby replacing whatever was set before.

If a user has a SchedulerFactoryBeanCustomizer and sets global job listeners, it should now be Sentry functionality that breaks, not user functionality. To get back the Sentry functionality SentryJobListener has to be added manually to the global job listeners by the user.

💡 Motivation and Context

Fixes #3088

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link
Contributor

github-actions bot commented Dec 13, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 5c8f9bf

Copy link
Contributor

github-actions bot commented Dec 13, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 404.04 ms 455.69 ms 51.65 ms
Size 1.72 MiB 2.27 MiB 558.21 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8838e01 387.41 ms 467.00 ms 79.59 ms
4e260b3 384.08 ms 477.56 ms 93.48 ms
93a76ca 377.41 ms 448.22 ms 70.81 ms
0bd723b 375.20 ms 452.41 ms 77.20 ms
86f0096 368.63 ms 446.92 ms 78.29 ms
0404ea3 332.47 ms 401.12 ms 68.66 ms
b172d4e 412.60 ms 492.68 ms 80.08 ms
93a76ca 397.30 ms 455.16 ms 57.87 ms
8ff8fd0 432.77 ms 495.18 ms 62.41 ms
7eccfdb 389.94 ms 461.29 ms 71.35 ms

App size

Revision Plain With Sentry Diff
8838e01 1.72 MiB 2.29 MiB 578.15 KiB
4e260b3 1.72 MiB 2.27 MiB 554.95 KiB
93a76ca 1.72 MiB 2.29 MiB 576.75 KiB
0bd723b 1.72 MiB 2.29 MiB 578.09 KiB
86f0096 1.72 MiB 2.29 MiB 576.50 KiB
0404ea3 1.72 MiB 2.29 MiB 577.52 KiB
b172d4e 1.72 MiB 2.29 MiB 578.09 KiB
93a76ca 1.72 MiB 2.29 MiB 576.75 KiB
8ff8fd0 1.72 MiB 2.27 MiB 558.15 KiB
7eccfdb 1.72 MiB 2.27 MiB 556.93 KiB

Previous results on branch: feat/quartz-customizer-order

Startup times

Revision Plain With Sentry Diff
af1565d 380.02 ms 433.42 ms 53.40 ms
cf101e0 385.29 ms 458.02 ms 72.73 ms

App size

Revision Plain With Sentry Diff
af1565d 1.72 MiB 2.27 MiB 554.95 KiB
cf101e0 1.72 MiB 2.27 MiB 554.95 KiB

Copy link
Collaborator

@lbloder lbloder left a comment

Choose a reason for hiding this comment

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

Code Changes look good 👍
Maybe we could add tests for this behavior?

Something like this:

    @Test
    fun `Sentry quartz job listener is added`() {
        contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj", "sentry.enable-automatic-checkins=true")
            .withUserConfiguration(QuartzAutoConfiguration::class.java)
            .run {
                val jobListeners = it.getBean(Scheduler::class.java).listenerManager.jobListeners
                assertThat(jobListeners).hasSize(1)
                assertThat(jobListeners[0]).matches(
                    { it.name == "sentry-job-listener" },
                    "is sentry job listener"
                )
            }
    }

    @Test
    fun `user defined SchedulerFactoryBeanCustomizer overrides Sentry Customizer`() {
        contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj", "sentry.enable-automatic-checkins=true")
            .withUserConfiguration(QuartzAutoConfiguration::class.java, CustomSchedulerFactoryBeanCustomizerConfiguration::class.java)
            .run {
                val jobListeners = it.getBean(Scheduler::class.java).listenerManager.jobListeners
                assertThat(jobListeners).hasSize(1)
                assertThat(jobListeners[0]).matches(
                    { it.name == "custom-job-listener" },
                    "is custom job listener"
                )
            }
    }


    @Configuration(proxyBeanMethods = false)
    open class CustomSchedulerFactoryBeanCustomizerConfiguration {
        class MyJobListener: JobListener {
            override fun getName() = "custom-job-listener"

            override fun jobToBeExecuted(context: JobExecutionContext?) {
                //do nothing
            }

            override fun jobExecutionVetoed(context: JobExecutionContext?) {
                //do nothing
            }

            override fun jobWasExecuted(
                context: JobExecutionContext?,
                jobException: JobExecutionException?
            ) {
                //do nothing
            }

        }
        @Bean
        open fun mySchedulerFactoryBeanCustomizer(): SchedulerFactoryBeanCustomizer {
            return SchedulerFactoryBeanCustomizer { schedulerFactoryBean -> schedulerFactoryBean.setGlobalJobListeners(MyJobListener()) }
        }
    }

Copy link
Collaborator

@lbloder lbloder left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@adinauer adinauer merged commit ec62a17 into main Dec 19, 2023
25 checks passed
@adinauer adinauer deleted the feat/quartz-customizer-order branch December 19, 2023 14:27
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.

SentrySchedulerFactoryBeanCustomizer overrides existing quartz listeners
2 participants