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 telemetry for basic Java system properties #8787

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Dec 17, 2023

This could be useful to collect. There is some overlap with the general usage stats (so not all of this will be new), and it doesn't get agent JVM information, but still. I think this tells us more about OS and JVM than what we currently know.

Testing done

No dedicated autotest due to the similarity with other similar telemetry implementations.

ExtensionList.lookupSingleton(jenkins.telemetry.impl.JavaSystemProperties).createContent()

Output:

Result: {"file.encoding":"UTF-8","file.separator":"/","java.vm.name":"OpenJDK 64-Bit Server VM","java.vm.vendor":"Eclipse Adoptium","java.vm.version":"11.0.15+10","os.arch":"aarch64","os.name":"Mac OS X","os.version":"13.6.1","user.language":"en","components":{"bouncycastle-api":"2.29","caffeine-api":"3.1.8-133.v17b_1ff2e0599","commons-lang3-api":"3.13.0-62.v7d18e55f51e2","commons-text-api":"1.10.0-78.v3e7b_ea_d5a_fe1","ionicons-api":"56.v1b_1c8c49374e","jackson2-api":"2.15.2-350.v0c2f3f8fc595","javax-activation-api":"1.2.0-6","jaxb":"2.3.9-1","jenkins-core":"2.436-SNAPSHOT","matrix-auth":"3.2.1","scm-api":"676.v886669a_199a_a_","script-security":"1275.v23895f409fb_d","snakeyaml-api":"2.2-111.vc6598e30cc65","structs":"325.vcb_307d2a_2782"}}

On Configure System, the help shows as expected:

system properties trial

Ran ExtensionList.lookupSingleton(jenkins.telemetry.Telemetry$TelemetryReporter).run() and confirmed it shows up in Uplink.

Added a test property to the list to ensure it renders correctly:

Screenshot 2023-12-17 at 23 23 29

Result: {"file.encoding":"UTF-8","file.separator":"/","java.vm.name":"OpenJDK 64-Bit Server VM","java.vm.vendor":"Eclipse Adoptium","java.vm.version":"11.0.15+10","os.arch":"aarch64","os.name":"Mac OS X","os.version":"13.6.1","test":"(undefined)","user.language":"en","components":{"bouncycastle-api":"2.29","caffeine-api":"3.1.8-133.v17b_1ff2e0599","commons-lang3-api":"3.13.0-62.v7d18e55f51e2","commons-text-api":"1.10.0-78.v3e7b_ea_d5a_fe1","ionicons-api":"56.v1b_1c8c49374e","jackson2-api":"2.15.2-350.v0c2f3f8fc595","javax-activation-api":"1.2.0-6","jaxb":"2.3.9-1","jenkins-core":"2.436-SNAPSHOT","matrix-auth":"3.2.1","scm-api":"676.v886669a_199a_a_","script-security":"1275.v23895f409fb_d","snakeyaml-api":"2.2-111.vc6598e30cc65","structs":"325.vcb_307d2a_2782"}}

Why even test for null? While I expect it to always exist, https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/System.html#getProperties() doesn't list file.encoding as part of the default set, so I added this to be safe.

Proposed changelog entries

  • Add telemetry for basic Java system properties describing the environment

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

@daniel-beck daniel-beck force-pushed the file.encoding-telemetry branch from 1a73ddc to 687145a Compare December 17, 2023 22:40
@@ -48,7 +48,7 @@
<j:if test="${not collector.end.isBefore(now)}">
<dt>${collector.displayName}</dt>
<dd>
<st:include from="${collector}" optional="true" page="description.jelly"/>
<st:include it="${collector}" optional="true" page="description.jelly"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

Required to be able to access it (the current Telemetry implementation) in description.jelly.

@timja timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Dec 18, 2023
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

LGTM

@daniel-beck daniel-beck added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Dec 18, 2023
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 18, 2023
@NotMyFault NotMyFault merged commit b0cec67 into jenkinsci:master Dec 19, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted squash-merge-me Unclean or useless commit history, should be merged only with squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants