-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Build time analytics #33555
Build time analytics #33555
Conversation
This comment has been minimized.
This comment has been minimized.
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
@snazy would you be able to double check my changes to the gradle plugin? |
This comment has been minimized.
This comment has been minimized.
Sure thing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a look, mostly from the Gradle plugin. I recommend to call the AnalyticsService
directly from the BuildWorker
, which should have everything that's needed. Doing so would also remove the need for the added tasks and the new extension. Having some Gradle IT(s) would be nice.
...gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/QuarkusAnalytics.java
Outdated
Show resolved
Hide resolved
.../gradle-application-plugin/src/main/java/io/quarkus/gradle/extension/AnalyticsExtension.java
Outdated
Show resolved
Hide resolved
.../gradle-application-plugin/src/main/java/io/quarkus/gradle/extension/AnalyticsExtension.java
Outdated
Show resolved
Hide resolved
...dent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/rest/RestClient.java
Show resolved
Hide resolved
...jects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientFailTest.java
Outdated
Show resolved
Hide resolved
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Show resolved
Hide resolved
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Show resolved
Hide resolved
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Show resolved
Hide resolved
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Outdated
Show resolved
Hide resolved
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Outdated
Show resolved
Hide resolved
Thanks @snazy. The review was much appreciated. |
...projects/tools/analytics-common/src/main/java/io/quarkus/analytics/config/GroupIdFilter.java
Show resolved
Hide resolved
...ts/tools/analytics-common/src/main/java/io/quarkus/analytics/dto/segment/TrackEventType.java
Outdated
Show resolved
Hide resolved
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logs when they show up in mvn/gradle output does not tell what is making the log/error - ie. quarkus usage analytics.
/cc @mjurc (qe) |
...endent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/ConfigService.java
Outdated
Show resolved
Hide resolved
...endent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/ConfigService.java
Outdated
Show resolved
Hide resolved
...endent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/ConfigService.java
Outdated
Show resolved
Hide resolved
rsvobodawould you also be able to test in gradle and also behind a proxy? |
...endent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/ConfigService.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through the Gradle changes and some related pieces of the code. The Gradle changes mostly LGTM, a couple mostly minor comments, except that there's no unit/integration test for Gradle.
One concern about the use of the fork-join-common-pool in ConfigService
.
Also added some suggestions (maybe not doable at this stage?) about the contents of the analytics data.
...adle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/worker/BuildWorker.java
Outdated
Show resolved
Hide resolved
...adle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/worker/BuildWorker.java
Outdated
Show resolved
Hide resolved
...ools/gradle/gradle-application-plugin/src/test/java/io/quarkus/gradle/QuarkusPluginTest.java
Outdated
Show resolved
Hide resolved
...ools/gradle/gradle-application-plugin/src/test/java/io/quarkus/gradle/QuarkusPluginTest.java
Show resolved
Hide resolved
devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/QuarkusPlugin.java
Outdated
Show resolved
Hide resolved
.pair(PROP_OS_ARCH, getProperty("os.arch", "N/A")) | ||
.build() | ||
.mapPair(PROP_CI) | ||
.pair(PROP_CI_NAME, getBuildSystemName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have a property that tells whether the CI
environment is set to indicate that it's running in CI (in case getBuildSystemName
returns unknown
for some exotic CI)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a limited number of CIs we can detect. It returns unknown when he couldn't detect any CI.
.pair(PROP_VERSION, getProperty("java.version", "N/A")) | ||
.build() | ||
.mapPair(PROP_GRAALVM) | ||
.pair(PROP_VENDOR, ofNullable(buildInfo.get(GRAALVM_VERSION_DISTRIBUTION)).orElse("N/A")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: wonder why not just Map.getOrDefault
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be less verbose but I'll not change that now to reduce the amount of changes.
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Show resolved
Hide resolved
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Show resolved
Hide resolved
...ent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/AnalyticsService.java
Show resolved
Hide resolved
Forgot to mention: the Gradle changes look MUCH simpler now - thanks for that! |
Thanks for your guidance @snazy! |
a8e053f
to
eff7783
Compare
Updated a field name on the remote payload |
eff7783
to
9061f37
Compare
This comment has been minimized.
This comment has been minimized.
Failing Jobs - Building 9061f37
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 17 Windows #- Failing: integration-tests/smallrye-context-propagation
📦 integration-tests/smallrye-context-propagation✖
|
@paulrobinson @maxandersen @rolfedh @rsvoboda @snazy @jsmrcka Do you have any concerns with the PR? If not, can you please approve? |
Sorry, was on pto. I don't capacity to look into this today. Not aware of any proxy I could use right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few comments and questions but I see nothing blocking.
Let's make sure we fix these issues (when they are actually issues) before the Final though.
Let me test it locally again and I'll give my approval.
devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/Prompter.java
Show resolved
Hide resolved
devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/QuarkusDev.java
Show resolved
Hide resolved
...adle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/worker/BuildWorker.java
Show resolved
Hide resolved
@Component | ||
BuildAnalyticsProvider analyticsProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component is automatically closed? I'm surprised it is given it doesn't implement any marker interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing is performed in the BootstrapSessionListener
* Build system details, such as Maven, Gradle, and so on. | ||
* If a CI system was detected | ||
* Country and timezone | ||
* Extensions enabled. Only extensions whose `groupIds` start with `io.quarkus`, `io.quarkiverse` or are included in the quarkus platform are collected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will rephrase to:
Only extensions whose groupIds
start with io.quarkus
, io.quarkiverse
and others included in the quarkus platform are collected.
} else { | ||
try { | ||
CompletableFuture<String> userInputFuture = CompletableFuture | ||
.supplyAsync(() -> analyticsEnabledSupplier.apply(ACCEPTANCE_PROMPT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geoand @cescoffier could you have a look at this?
"io.quarkus", | ||
"io.quarkiverse", | ||
"org.apache.camel.quarkus", | ||
"io.debezium", | ||
"org.drools", | ||
"org.optaplanner", | ||
"org.amqphub.quarkus", | ||
"com.hazelcast", | ||
"com.datastax.oss.quarkus"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so you need to clarify the list in the documentation because we are not doing what we documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are group Ids of extensions bundled on the platform.
On a follow up PR:
|
@rsvoboda Thanks very much. Will debug and fix the windows issue in a new PR. In relation to the proxy. My main concern was if it would hang the build or blow up in some unforeseen way. Proxy handling should be a concern for later. |
I tried the builds with disconnected network, nothing was delayed or hanging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge and iterate from there.
This PR enables an opt-in feature to allow gathering of build time analytics to get info/insights about how Quarkus is being used during development/builds (only build time NOT runtime).
TO TEST THIS
build the PR first using
mvn -Dquickly
, then create a project using latest snapshot and with a non quarkus grouped, for example:quarkus create app -P io.quarkus:quarkus-bom:999-SNAPSHOT something.random:myapp
.Please try and use your own groupid/name so we get more diverse reports.
Then run
quarkus dev
in the project and sayy
to report data.Running in dev mode and mvn quarkus:dev, and gradle quarkusDev should report then submit data on each run.
Check
target/build-analytics-event.json
for content of what was sent.Implementation
It is implemented using Segment which is used by other Red Hat projects to gather anonymous usage data.
Intent is that on first quarkus dev mode user will be asked if they wish to share anonymously build time info to Quarkus team.
If answer is no, marker file is created in
~/.redhat
and no recording/analytics is made before that markerfile is removed.If answer is yes, markerfile is made +
~/.redhat/anonymousId
is created which is just to be able to group data per installation without any identifiable info in them. From that point on when ever quarkus build plugin runs it will send a json packet with info. This is sent async and non-blocking. Meaning build won't block because of this.i.e. group-ids "io.quarkus", "io.quarkiverse", “org.apache.camel.quarkus” or listed in Quarkus platform.
An example of data collected is shown below - all non-identifiable info.