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

Only disable analytics for Quarkus CLI dev or build command #1319

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Sep 16, 2024

Summary

This PR does 2 things:

  • disables analytics for remote dev mode as we waste time there (10 seconds, see daily builds)
  • refactors workaround for TLS command that disables analytics just for itself

Right now analytics are send during build and DEV mode. In https://quarkus.io/usage/ and https://quarkus.io/version/main/guides/build-analytics#show-me-the-code Quarkus explicitly documents that:

  • analytics are send in DEV mode
  • you can look when analytics are send in io.quarkus.analytics.AnalyticsService#sendAnalytics:
    • I checked an they are also send for quarkus build command

This can change in the future and noone from QE team will check every one and then if that has changed. So until now we always set -Dquarkus.analytics.disabled=true unless explicitly disabled. Now it does matter when Quarkus CLI TLS command is used because that command reports unknown option for setting this system property.

What are our options:

  • report this inconsistency in Quarkus project and do nothing till it's addressed (if it is addressed)
    • but that means reporting something that is not clear bug, just our incovenience; for example, Quarkus supports "CI" environment variable that allows to disable this behavior, but we would have to change all our Jenkins jobs / GH workflows
  • only exclude TLS command
    • that is my favorite, because it isolates scenario that clearly doesn't need analytics but doesn't break anything
  • do what this PR does - only disable analytics when really needed

Honestly, not sure what is best, I think one day someone will report that I slowed down CI, but on the other hand, doing what this PR does, we won't always disable analytics when not needed. Why should users always do that? It feels stupid to expect users to always add it and I don't believe they will. So closer to user case.

Update: I rerun related tests and this seems to disable analytics as expected.

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Release (follows conventions described in the RELEASE.md)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@michalvavrik michalvavrik changed the title Only disable analytics for CLI dev or build cmd Only disable analytics for Quarkus CLI dev or build command Sep 16, 2024
@michalvavrik michalvavrik marked this pull request as draft September 16, 2024 09:24
@michalvavrik michalvavrik marked this pull request as ready for review September 16, 2024 09:34
@michalvavrik michalvavrik force-pushed the feature/refactor-disabling-of-cli-analytics branch 3 times, most recently from a48de33 to 793a4ad Compare September 16, 2024 09:52
@michalvavrik michalvavrik force-pushed the feature/refactor-disabling-of-cli-analytics branch from 793a4ad to 24a0285 Compare September 16, 2024 10:03
@michalvavrik michalvavrik requested review from rsvoboda and removed request for rsvoboda September 16, 2024 11:14
@michalvavrik
Copy link
Member Author

michalvavrik commented Sep 16, 2024

I think @rsvoboda is busy (after reading that next build arrived in my inbox) so I'll set @mjurc , but please @rsvoboda feel free to comment. We can always tweak things later.

@michalvavrik michalvavrik requested a review from mjurc September 16, 2024 20:17
Copy link
Member

@mjurc mjurc left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for the fix

@mjurc mjurc merged commit 3ba4289 into quarkus-qe:main Sep 17, 2024
8 checks passed
@michalvavrik michalvavrik deleted the feature/refactor-disabling-of-cli-analytics branch September 17, 2024 14:15
@michalvavrik michalvavrik mentioned this pull request Sep 17, 2024
11 tasks
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.

2 participants