issues/1597: Temperature Monitoring #1876
Conversation
…s object for NodeJs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1876 +/- ##
==========================================
+ Coverage 46.39% 46.80% +0.40%
==========================================
Files 954 962 +8
Lines 59770 60554 +784
Branches 5537 5705 +168
==========================================
+ Hits 27730 28341 +611
- Misses 31921 32094 +173
Partials 119 119 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
api/src/unraid-api/graph/resolvers/metrics/temperature/sensors/ipmi_sensors.service.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts`:
- Around line 208-259: After mapping/filtering sensors, guard the fast-path
against empty sensor sets by checking sensors.length and returning early with
the same response shape but summary set to null (e.g. id 'temperature-metrics',
sensors: [], summary: null) instead of calling buildSummary; this prevents
buildSummary from throwing on an empty array. Apply the same early-return guard
wherever the same fast-path occurs (see the other occurrence around the
buildSummary call at the later block referenced by lines 369-372). Ensure you
reference the sensors variable and buildSummary function when adding the check.
- Around line 129-133: Replace the unsafe enum lookup and any-typed threshold
params by adding a typed parser and a TemperatureThresholdConfig type, then use
them: implement a private parseUnit(configUnit: string): TemperatureUnit that
converts configUnit.toUpperCase() safely to a TemperatureUnit (falling back to
TemperatureUnit.CELSIUS) and replace occurrences of (TemperatureUnit as
any)[configUnit.toUpperCase()] with this.parseUnit(configUnit); define an
interface/type TemperatureThresholdConfig for the thresholds shape and change
the methods that accept thresholdConfig/thresholds to use that type (e.g.,
update the parameter signatures referenced as thresholdConfig and thresholds in
the class to TemperatureThresholdConfig).
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts (2)
21-21: Remove the redundant file header comment.It doesn’t add clarity and conflicts with the no-unnecessary-comments guideline.
♻️ Proposed change
-// temperature.service.tsAs per coding guidelines, "Never add comments unless they are needed for clarity of function".
154-154: Use function overloads to avoid type assertions onconvertReadingcalls.Remove the
as TemperatureReadingcasts at lines 154 and 218 by adding overloads to theconvertReadingmethod. This eliminates unnecessary type assertions while maintaining full type safety:♻️ Suggested overloads
-private convertReading( - reading: TemperatureReading | undefined, - targetUnit: TemperatureUnit -): TemperatureReading | undefined { +private convertReading( + reading: TemperatureReading, + targetUnit: TemperatureUnit +): TemperatureReading; +private convertReading( + reading: TemperatureReading | undefined, + targetUnit: TemperatureUnit +): TemperatureReading | undefined; +private convertReading( + reading: TemperatureReading | undefined, + targetUnit: TemperatureUnit +): TemperatureReading | undefined { if (!reading) return undefined; return { ...reading, value: this.convertValue(reading.value, reading.unit, targetUnit), unit: targetUnit, }; }-const current = this.convertReading(rawCurrent, targetUnit) as TemperatureReading; +const current = this.convertReading(rawCurrent, targetUnit);-const current = this.convertReading(rawCurrent, targetUnit) as TemperatureReading; +const current = this.convertReading(rawCurrent, targetUnit);Also applies to line 218.
api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.spec.ts`:
- Around line 25-53: The test currently uses unsafe as any casts for mocks
(lmSensors, diskSensors, ipmiSensors, configService); replace those with
properly typed mocks by declaring the sensor mocks as
Partial<TemperatureSensorProvider> objects implementing the needed methods
(isAvailable, read) and by instantiating a real ConfigService and using
vi.spyOn(configService, 'get') to control return values instead of casting to
any—update references in the spec to use the typed
Partial<TemperatureSensorProvider> mocks and the spied ConfigService so the
compiler enforces correct method signatures while keeping the same mock
behavior.
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts (1)
21-38: Trim redundant inline comments.
The file/constructor comments restate the code intent; removing them keeps the file aligned with the no-comment guideline.As per coding guidelines:
**/*.{ts,tsx,js,jsx}: Never add comments unless they are needed for clarity of function.
api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@api/docs/developer/temperature.md`:
- Line 18: The table entry for history.max_readings currently lists a default of
1000 but the sample config uses 144; fix the inconsistency by either updating
the table default for history.max_readings to 144 to match the sample, or keep
the table at 1000 and add a clear inline comment next to the sample
configuration showing that the sample intentionally overrides the default (e.g.,
"override for low-memory devices: 144") so readers understand the deviation;
locate references to history.max_readings in the docs and make the change in the
table row and/or the sample block accordingly.
🧹 Nitpick comments (1)
api/docs/developer/temperature.md (1)
18-19: Clarify or remove "(Internal)" designation.Both
history.max_readingsandhistory.retention_msare marked as "(Internal)" in the description, yet they appear as user-configurable settings in the sample configuration (lines 60-62). This is confusing.If these settings are configurable by users, remove the "(Internal)" label. If they're meant to be internal-only, explain why they're documented in user-facing configuration documentation.
…d some comments for luddites like myself
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts`:
- Line 238: Replace the non-null assertion on metadata from
this.history.getMetadata(sensorId)! with a defensive check: store the result in
a const (e.g., const metadata = this.history.getMetadata(sensorId)); if metadata
is null/undefined, handle it explicitly (return early, throw a clear error, or
fallback behavior) before using metadata; ensure references to metadata in the
surrounding method (the variable named metadata and the sensorId parameter) are
guarded so no runtime exception can occur if history was cleared.
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts (1)
30-30: Remove unnecessary file name comment.This comment adds no value since the file name is already visible in the file path.
🧹 Suggested fix
-// temperature.service.ts `@Injectable`() export class TemperatureService implements OnModuleInit {
api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts
Outdated
Show resolved
Hide resolved
…date all the typings consistently
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts (1)
412-417:⚠️ Potential issue | 🟡 MinorInconsistent use of
anytype.Line 308 and 388 were correctly updated to use
unknown, but this occurrence was missed.Suggested fix
- vi.mocked(configService.get).mockImplementation((key: string, defaultValue?: any) => { + vi.mocked(configService.get).mockImplementation((key: string, defaultValue?: unknown) => {As per coding guidelines: "Never use the
anytype. Always prefer proper typing."
🤖 Fix all issues with AI agents
In
`@api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.spec.ts`:
- Around line 430-447: Remove the stale inline comment ("// Document expected
behavior...") in the test and update the assertion to match the current behavior
where non-finite values are filtered: after calling service.onModuleInit(),
mocking lmSensors.read() to return the NaN sensor, assert via
service.getMetrics() that the returned metrics do not include the 'nan-sensor'
entry (or assert the returned metrics array is empty) instead of leaving the
misleading comment or expecting metrics toBeNull; locate this change around the
test that mocks lmSensors.read!, the onModuleInit() call, and the getMetrics()
assertion.
🧹 Nitpick comments (5)
api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts (1)
24-25: Remove unnecessary comment.This empty comment section adds no value and violates the coding guideline about avoiding unnecessary comments.
Suggested fix
-// Vitest imports - // Mock the external dependencies using viAs per coding guidelines: "Never add comments unless they are needed for clarity of function."
api/src/unraid-api/graph/resolvers/metrics/temperature/sensors/disk_sensors.service.spec.ts (2)
37-75: Avoidas unknown as Diskfixtures.
The double-casts bypass type safety. Prefer a typed disk fixture factory (orsatisfies Disk) so tests stay aligned with the model.As per coding guidelines: Avoid using casting whenever possible, prefer proper typing from the start.
141-170: Align test name with the interface type under test.
The “nvme interface” case currently usesDiskInterfaceType.PCIE. If an NVME enum exists, add an explicit case; otherwise rename the test to PCIE for clarity.api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts (2)
30-63: Trim redundant comments.
The file/section headers restate the code and aren’t adding clarity; removing them keeps the file cleaner.As per coding guidelines: Never add comments unless they are needed for clarity of function.🧹 Suggested cleanup
-// temperature.service.ts `@Injectable`() export class TemperatureService implements OnModuleInit { @@ - // Inject all available sensor providers private readonly lmSensors: LmSensorsService, private readonly diskSensors: DiskSensorsService, private readonly ipmiSensors: IpmiSensorsService, @@ - // Initialize all providers and check availability await this.initializeProviders();
173-176: Avoidas TemperatureReadingcasts via overloads.
You can overloadconvertReadingso callers with defined readings get a non-optional return.As per coding guidelines: Avoid using casting whenever possible, prefer proper typing from the start.♻️ Suggested refactor
- const current = this.convertReading(rawCurrent, targetUnit) as TemperatureReading; + const current = this.convertReading(rawCurrent, targetUnit); @@ - const current = this.convertReading(rawCurrent, targetUnit) as TemperatureReading; + const current = this.convertReading(rawCurrent, targetUnit); @@ - private convertReading( - reading: TemperatureReading | undefined, - targetUnit: TemperatureUnit - ): TemperatureReading | undefined { + private convertReading( + reading: TemperatureReading, + targetUnit: TemperatureUnit + ): TemperatureReading; + private convertReading( + reading: TemperatureReading | undefined, + targetUnit: TemperatureUnit + ): TemperatureReading | undefined { if (!reading) return undefined; return { ...reading, value: this.convertValue(reading.value, reading.unit, targetUnit), unit: targetUnit, }; }Also applies to: 242-244, 287-298
api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.spec.ts
Show resolved
Hide resolved
1 participant
Temperature Monitoring System Feature Request
Description
This PR implements a comprehensive temperature monitoring system integrated into the existing
MetricsResolver. It provides real-time access to temperature data from various system sensors via GraphQL queries and subscriptions.I tested these changes locally using the provided Apollo Server on Unraid
7.2.0.Addresses #1597
Key Features
lm-sensorsintegration.smartctlintegrationsmartctlto leverage json parsing instead of raw strings.ipmitoolsensors (see notes below!).temperaturefield on theMetricsnode.systemMetricsTemperaturesubscription for real-time updates.current,min,max, andhistoryfor each sensor.api.json(enabled status, polling intervals, thresholds).default_unitpreference (Celsius/Fahrenheit) with automatic conversion.Implementation Details
sensors,smartctl,ipmitool) rather than bundling binaries, aligning with the base OS integration strategy. There is no attempt here to package sensor tooling as part of this feature. This is per a conversation on the feature scope with the Unraid team.TemperatureSensorProviderinterface allowing for easy addition of new sensor types.DisksServicewas updated to parsesmartctlJSON output directly, resolving issues with raw string parsing on certain drive models. There was an issue with some Seagate drives reporting raw values with extra data in parentheses:24 (0 14 0 0 0). Parsing the last value in the string returned incorrect data. Parsing with json prevents this.Documentation
docs/developer/temperature.mddetailing configuration options and API usage.Scope Notes
IpmiSensorsServicehas been implemented to parse standardipmitooloutput, but it has not been tested on live hardware as I do not have access to a system with IPMI support. It relies on standardipmitooloutput formats (at least the documentation I saw and what AI told me).WARNINGandCRITICALstatuses based on thresholds but does not currently trigger active system notifications. This type of alert is passive only.Summary by CodeRabbit
New Features
Documentation
Tests