feat: add telemetry subcrate #15

nici · 2025-04-28T19:49:35Z

nici commented

2025-04-28 19:49:35 +00:00

First-time contributor

No description provided.

nici added 1 commit 2025-04-28 19:49:37 +00:00

feat: add telemetry subcrate

Build Zenyx ⚡ / 🧪 Run Cargo Tests (pull_request) Failing after 4m55s

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-apple-darwin (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-pc-windows-msvc (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-unknown-linux-gnu (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-apple-darwin (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-pc-windows-msvc (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-unknown-linux-gnu (pull_request) Has been skipped

Details

91c80d0f91

lily requested changes 2025-04-28 20:04:30 +00:00

src/lib.rs

					
				@ -1 +1 @@

				include!("main.rs");

				include!("main.rs");

lily commented

2025-04-28 19:54:52 +00:00

Why is this even being changed?

subcrates/telemetry/src/languages.json Outdated

					
				@ -0,0 +1,684 @@

				{

lily commented

2025-04-28 19:56:39 +00:00

Again, this can be inlined.

bitsyndicate commented

2025-04-28 20:14:32 +00:00

probably shouldn't tho.

include_str!("path")

as to not pollute the source file

probably shouldn't tho. ```rust include_str!("path") ``` as to not pollute the source file

👍 2

subcrates/telemetry/src/lib.rs Outdated

					
				@ -0,0 +36,4 @@

				    #[derive(Debug, Serialize, Default)]

				    pub struct DisplayInfo {

				        resolution: String,

				        refresh_rate: String,

lily commented

2025-04-28 19:57:32 +00:00

Nit, this could be a float instead.

bitsyndicate commented

2025-04-28 20:11:42 +00:00

Or even an int...

lily commented

2025-04-28 20:20:10 +00:00

I suggested a float as some monitors run at strange refresh rates, such as 59.97 hz, so a float would be the only way to store that as a non-string value.

I suggested a float as some monitors run at *strange* refresh rates, such as 59.97 hz, so a float would be the only way to store that as a non-string value.

bitsyndicate commented

2025-04-28 20:30:21 +00:00

ah fair

subcrates/telemetry/src/lib.rs Outdated

					
				@ -0,0 +332,4 @@

				        edition: String,

				        version: String,

				        architecture: String,

				        kernel: String,

lily commented

2025-04-28 19:59:41 +00:00

kernel and de should be optional as kernel is only applicable on macOS and Linux, and de is only applicable on Linux.

`kernel` and `de` should be optional as `kernel` is only applicable on macOS and Linux, and `de` is only applicable on Linux.

subcrates/telemetry/src/lib.rs Outdated

					
				@ -0,0 +346,4 @@

				            } else if cfg!(target_pointer_width = "32") {

				                String::from("32 bit")

				            } else {

				                String::from("16 bit")

lily commented

2025-04-28 20:01:08 +00:00

16-bit is not supported, I'd rather this just say "unknown" or something similar.

subcrates/telemetry/src/lib.rs Outdated

					
				@ -0,0 +621,4 @@

				    pub fn format_duration(duration: StdDuration) -> String {

				        let mut parts = Vec::new();

				        let mut remaining = duration.as_secs();

				        let units = [

lily commented

2025-04-28 20:04:06 +00:00

This should be done with ~~chrono~~¹ timeago instead.

chrono does not support relative time, use the suggested crate instead, it's quite minimal. ↩︎

This should be done with ~~`chrono`~~[^1] `timeago` instead. [^1]: `chrono` does not support relative time, use the suggested crate instead, it's quite minimal.

bitsyndicate commented

2025-04-28 20:57:32 +00:00

Chrono supports relative time. That's what chrono::Duration is for. To format it, one can simply use the serialize or display implementation which return the duration in ISO8601 impl Display for TimeDelta (Duration is just an alias for TimeDelta)

Chrono supports relative time. That's what [`chrono::Duration`](https://docs.rs/chrono/latest/chrono/type.Duration.html) is for. To format it, one can simply use the serialize or display implementation which return the duration in ISO8601 [`impl Display for TimeDelta`](https://docs.rs/chrono/latest/chrono/type.Duration.html#impl-Display-for-TimeDelta) (`Duration` is just an alias for `TimeDelta`)

lily commented

2025-04-28 21:02:10 +00:00

If only the docs were actually properly searchable 🤦‍♀️

subcrates/telemetry/src/main.rs

					
				@ -0,0 +1,3 @@

				fn main() {

lily commented

2025-04-28 19:54:20 +00:00

This file should not be in the subcrate.

subcrates/telemetry/src/territories.json Outdated

					
				@ -0,0 +1,318 @@

				{

lily commented

2025-04-28 19:55:37 +00:00

This could be inlined as a static or done some other way that doesn't require this file.

bitsyndicate commented

2025-04-28 20:16:08 +00:00

again inline_str!()

again `inline_str!()`

👍 1

lily added this to the Zenyx project 2025-04-28 20:05:40 +00:00

lily

2025-04-28 20:06:14 +00:00

added the
Kind/Feature

Priority
Low

Platform
All
labels
requested reviews from caznix, bitsyndicate

bitsyndicate requested changes 2025-04-28 20:17:37 +00:00

Dismissed

bitsyndicate left a comment

I agree with lily on almost all. Although I would request you to create seperate files for the modules and only use inline modules for tests etc. as unnecessarily long files are difficult to work with

bitsyndicate requested changes 2025-04-28 20:27:24 +00:00

bitsyndicate left a comment

Maybe also fix the weird UTF-8 bugs in the json dumps

lily commented

2025-04-28 20:32:54 +00:00

Please fix your Git identity, the email associated with your commits is not the one linked to your account. Additionally, like BitSyndicate said, please separate this into multiple files and multiple commits.

caznix requested changes 2025-04-28 21:30:38 +00:00

caznix left a comment

Structs are generally not type safe, and have finite values that are being ignored. Examples can be found on these lines:

22..27

31..34

36..40

210..217

278..284

286..291

330..336

370..377

410..413

608..612

as a specific example, looking at the structs in the gpu module DriverInfo, AdapterInfo, GPUInfo, and DisplayInfo, they use strings for known uint values and cannot be logically compared:

    #[derive(Debug, Serialize)]
    pub struct AdapterInfo {
        vendor: String,
//      ^^^^^
//      There is a known and finite list of vendors.
        model: String,
        driver: DriverInfo,
        vram: String,
        display: DisplayInfo,
    }
    #[derive(Debug, Serialize, Default)]
    pub struct DisplayInfo {
        resolution: String,
//      ^^^^^^^^^^
//      This is a known integer and could be wrapped
//      resolution: (u64,u64)
        refresh_rate: String,
//      ^^^^^^^^^^^^
//      This is just a float, an f32 would be
//      generally large enough to fit
//      modern refresh rates
    }

As a more intricate suggestion, making use of the Versions crate would be ideal for this use-case. To quote their example:

let good = Versioning::new("1.6.0").unwrap();
let evil = Versioning::new("1.6.0a+2014+m872b87e73dfb-1").unwrap();

assert!(good.is_ideal());   // It parsed as a `SemVer`.
assert!(evil.is_complex()); // It parsed as a `Mess`.
assert!(good > evil);       // We can compare them anyway!

Other than the type safety, the code makes many assumptions via .unwrap() and .expect() calls, instead of having fallible functions.

In addition, the code also lacks documentation and tests, even in potentially ambigous areas, for example:

fn vendor_name(vendor: u32) -> &'static str {
    match vendor {
        0x10DE => "NVIDIA",
        0x1002 => "AMD(Advanced Micro Devices), Inc.",
        0x8086 => "Intel(integrated electronics)",
        0x13B5 => "ARM(Advanced RISC Machines)",
        0x5143 => "Qualcomm(Quality Communications)",
        0x1010 => "Apple Inc.",
        _ => "Unknown",
    }
}

Without context, anyone reading this would not know that vendors have predetermined ID's assigned to them, which should be clarified.

Due to the former, I wouldn't feel comfortable using this crate in the main engine quite yet. If these issues are corrected, this PR will be reconsidered for merging.

Structs are generally not type safe, and have finite values that are being ignored. Examples can be found on these lines: 22..27 31..34 36..40 210..217 278..284 286..291 330..336 370..377 410..413 608..612 as a specific example, looking at the structs in the gpu module `DriverInfo`, `AdapterInfo`, `GPUInfo`, and `DisplayInfo`, they use strings for known uint values and cannot be logically compared: ```rust #[derive(Debug, Serialize)] pub struct AdapterInfo { vendor: String, // ^^^^^ // There is a known and finite list of vendors. model: String, driver: DriverInfo, vram: String, display: DisplayInfo, } #[derive(Debug, Serialize, Default)] pub struct DisplayInfo { resolution: String, // ^^^^^^^^^^ // This is a known integer and could be wrapped // resolution: (u64,u64) refresh_rate: String, // ^^^^^^^^^^^^ // This is just a float, an f32 would be // generally large enough to fit // modern refresh rates } ``` As a more intricate suggestion, making use of the [`Versions`](https://crates.io/crates/versions) crate would be ideal for this use-case. To quote their example: ```rust let good = Versioning::new("1.6.0").unwrap(); let evil = Versioning::new("1.6.0a+2014+m872b87e73dfb-1").unwrap(); assert!(good.is_ideal()); // It parsed as a `SemVer`. assert!(evil.is_complex()); // It parsed as a `Mess`. assert!(good > evil); // We can compare them anyway! ``` Other than the type safety, the code makes many assumptions via `.unwrap()` and `.expect()` calls, instead of having fallible functions. In addition, the code also lacks documentation and tests, even in potentially ambigous areas, for example: ```rust fn vendor_name(vendor: u32) -> &'static str { match vendor { 0x10DE => "NVIDIA", 0x1002 => "AMD(Advanced Micro Devices), Inc.", 0x8086 => "Intel(integrated electronics)", 0x13B5 => "ARM(Advanced RISC Machines)", 0x5143 => "Qualcomm(Quality Communications)", 0x1010 => "Apple Inc.", _ => "Unknown", } } ``` Without context, anyone reading this would not know that vendors have predetermined ID's assigned to them, which should be clarified. Due to the former, I wouldn't feel comfortable using this crate in the main engine quite yet. If these issues are corrected, this PR will be reconsidered for merging.

👍 3

nici added 1 commit 2025-05-01 23:15:14 +00:00

removed languages.json and territories.json

Build Zenyx ⚡ / 🧪 Run Cargo Tests (pull_request) Failing after 6m9s

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-apple-darwin (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-pc-windows-msvc (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-unknown-linux-gnu (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-apple-darwin (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-pc-windows-msvc (pull_request) Has been skipped

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-unknown-linux-gnu (pull_request) Has been skipped

Details

528d4b03a3

lib.rs is separated into src/modules now for cleaner code

optimized error handling to -> (storage.rs, uptime.rs, meta.rs, memory.rs, os.rs, network.rs)
optimized struct types to -> (storage.rs, uptime.rs, meta.rs, memory.rs, os.rs, network.rs)

Build Zenyx ⚡ / 🧪 Run Cargo Tests (pull_request) Failing after 6m9s

Required

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-apple-darwin (pull_request) Has been skipped

Required

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-pc-windows-msvc (pull_request) Has been skipped

Required

Details

Build Zenyx ⚡ / 🏗️ Build aarch64-unknown-linux-gnu (pull_request) Has been skipped

Required

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-apple-darwin (pull_request) Has been skipped

Required

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-pc-windows-msvc (pull_request) Has been skipped

Required

Details

Build Zenyx ⚡ / 🏗️ Build x86_64-unknown-linux-gnu (pull_request) Has been skipped

Required

Details

This pull request has changes conflicting with the target branch.

Cargo.lock

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u telemetry:nici-telemetry

git checkout nici-telemetry

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

4 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: nonsensical-dev/zenyx-engine#15

				`@ -0,0 +1,684 @@`
				`{`

				`@ -0,0 +1,318 @@`
				`{`

Rows
Columns