feat: extended http-api for providing all module variables as json …#3743
feat: extended http-api for providing all module variables as json …#3743JohannesRegneri wants to merge 5 commits intobitfocus:mainfrom
Conversation
|
|
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
Exciting, a few opinion points as an API consumer/trying to think of the future a bit...
companion/lib/Service/HttpApi.ts
Outdated
| this.#apiRouter.get('/variables/:label/json', this.#variablesGetJson) | ||
|
|
||
| // Prometheus metrics | ||
| this.#apiRouter.get('/variables/:label/metrics', this.#variablesGetMetrics) |
There was a problem hiding this comment.
I'd argue this should be Prometheus so other formats have a sensible namespace too (or maybe they should be further nested under metrics:
| this.#apiRouter.get('/variables/:label/metrics', this.#variablesGetMetrics) | |
| this.#apiRouter.get('/variables/:label/prometheus', this.#variablesGetPrometheus) |
E.g. if someone wanted to implement InfluxDB line protocol ( https://docs.influxdata.com/influxdb3/cloud-serverless/reference/syntax/line-protocol/ ) they could do it at:
/variables/:label/influx
companion/lib/Service/HttpApi.ts
Outdated
| this.#apiRouter.post('/surfaces/rescan', this.#surfacesRescan) | ||
|
|
||
| // JSON | ||
| this.#apiRouter.get('/appinfo/json', this.#appInfoGetJson) |
There was a problem hiding this comment.
Should this be app-info to match custom-variable above?
| this.#apiRouter.get('/appinfo/json', this.#appInfoGetJson) | |
| this.#apiRouter.get('/app-info/json', this.#appInfoGetJson) |
companion/lib/Service/HttpApi.ts
Outdated
| } | ||
|
|
||
| if (Object.keys(result).length == 0) { | ||
| res.status(404).json({ |
There was a problem hiding this comment.
Should no variables be a 404? I guess the real question is can we differentiate between a module with no variables and an invalid name for a module? If we can, I'd say it shouldn't be a 404 (and maybe it's just an empty response). Especially given module variables can be dynamic.
There was a problem hiding this comment.
The existing does return a 404, but only for a single variable, I'd argue here the status should be representing the module now (and whether it exists):
companion/companion/lib/Service/HttpApi.ts
Lines 634 to 635 in d829a13
| @@ -320,6 +320,14 @@ export class ServiceHttpApi { | |||
| // surfaces | |||
| this.#apiRouter.post('/surfaces/rescan', this.#surfacesRescan) | |||
There was a problem hiding this comment.
Adding reporting of surface state (e.g. offline) would also be really nice, but maybe that's a separate issue/PR?
companion/lib/Service/HttpApi.ts
Outdated
| ? this.#serviceApi.getCustomVariableValue(variableName) | ||
| : this.#serviceApi.getConnectionVariableValue(connectionLabel, variableName) | ||
|
|
||
| if (value !== undefined) { |
There was a problem hiding this comment.
Aren't undefined ones also valuable? I think some of my module ones are set to undefined (rather than the empty string) when they've got no data? I don't know if there's a recommended Companion policy there?
There was a problem hiding this comment.
I see the existing code does it for a single variable:
companion/companion/lib/Service/HttpApi.ts
Lines 634 to 635 in d829a13
Although maybe the distinction there is you know you've tried and it's either undefined or you've used the wrong name, here's it's just randomly missing (as you can't tell you've tried it).
companion/lib/Service/HttpApi.ts
Outdated
| : this.#serviceApi.getConnectionVariableValue(connectionLabel, variableName) | ||
|
|
||
| if (value !== undefined) { | ||
| result[variableName] = { |
There was a problem hiding this comment.
I don't know the right answer here, but in the Prometheus endpoint, you've included the connection name too.
If I've got multiple connections, talking to the same type of device (e.g. 3 Hyperdecks from the original issue), without the "namespace" being embedded in the returned JSON, I've got to do some processing before I can store them (or store by "namespace"), whereas I could just chuck all the Prometheus datapoints in a file and they're still unique. Although I appreciate it's more data being transmitted that's duplicated if I'm using them separately.
There was a problem hiding this comment.
I think that in general it is reasonable to not include the connection prefix, as that will make writing generic processing/parsing code simpler without having to make that parsing aware of the connection label.
In your case of working with multiple, it sounds reasonable to have to do a little bit of namespacing in your app, you may want to use different labels/ids too.
Prometheus is different, as it is expected they will be put into a global store, where it is likely that other connections/data will exist, so they need to be scoped. (although maybe that could be added by the scraper rule, so maybe not necessary to be builtin?)
There was a problem hiding this comment.
Fair enough, could we compromise on at least including the connection prefix once in a "header/request" type space, then consumers don't need to track the request/response pair to know what bit of data it was. Then even Companion's Generic HTTP module could process it! 😆
Prometheus is different, as it is expected they will be put into a global store, where it is likely that other connections/data will exist, so they need to be scoped. (although maybe that could be added by the scraper rule, so maybe not necessary to be builtin?)
I don't know enough about Prometheus, is the store per host being monitored, or global to the Prometheus instance? Obviously if it's the latter you've still got to add context as soon as you're checking two different Companion instances with the same type of connection configured on both (unless you've already made the connection names globally unique there).
There was a problem hiding this comment.
I think I'm agreeing with you on this endpoint not including connection label. Prometheus wont understand json though, so is a parallel discussion
Its a global store. As an example, if I query my prometheus with node_boot_time_seconds{}, I get back:
node_boot_time_seconds{instance="10.0.0.1:9100", job="node-a"} = 1763253353.944076
node_boot_time_seconds{instance="10.0.0.2:9100", job="node-b"} = 1763252538
node_boot_time_seconds{instance="10.0.0.3:9100", job="node-c"} = 1763237998
node_boot_time_seconds{instance="10.0.0.4:9100", job="node-d"} = 1762635123.979674
node_boot_time_seconds{instance="10.0.0.5:9100", job="node-e"} = 1762635247
I believe that in this case, both the instance and job are being injected by the scraping and are not in the source. But the source file is also able to specify other fields, eg node_cpu_seconds_total{} returns values looking like: node_cpu_seconds_total{cpu="0", instance="10.0.0.1:9100", job="node-a", mode="idle"} = 0
But the bit that worries me is a module producing a metric called simply ip pollutes the global store quite a bit, so they want to have better names in some way. Those two metrics I showed are from the node exporter (linux system), general convention appears to be to at least prefix it with companion_, but even that will still make quite a mush. Maybe the best thing would be companion_connection_${moduleType}_variable_${variableName} as that is probabyl closest to the typical structure.
That would avoid embedding the label in the name, while also allowing the metrics to have accurate descriptions (if we ignore that different versions of a module could vary)
companion/lib/Resources/Util.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * @returns Seconds since Jan 01 1970. (UTC) |
There was a problem hiding this comment.
Out of interest, what's the thinking behind this rather than say ISO format?
companion/lib/Service/HttpApi.ts
Outdated
| * Provides Prometheus metrics for module or custom variables | ||
| * | ||
| */ | ||
| #variablesGetMetrics = (req: Express.Request, res: Express.Response): void => { |
There was a problem hiding this comment.
I'm not keen on the structure of these metrics. As an occasional prometheus user, I would find these hard to work with.
Them being called something like custom_myvar_string will make them very hard to find when in a prometheus with other data. At the very least that should be prefixed with companion_.
But also I want to question if they should be called simply companion_variable, with the connection label and variable name being supplied as attributes/properties/whatever they are called.
My reason for this is that if you want to do any graphing based on a pattern (perhaps list them all in a table or just all with the name starting abc), that will be very hard to do with them as separate metrics, but if they are the same metric with different properties, then they can be easily queried together while still being easy to target down to the ones you want.
There was a problem hiding this comment.
Again I don't know enough about Prometheus, but thinking of something I'm aiming to do with a different monitoring solution is extracting health info like temperature from connection variables (across multiple devices of the same type) and graphing that. I assume a use case like that would still work with @Julusian suggestion?
Would you want to add the connection type too to the attributes, so you can graph all the FooBar 5000 temperatures together (without needing to know how each connection is labelled)?
There was a problem hiding this comment.
I think my answer #3743 (comment) covers some of this.
I wrote that before reading this, but yeah I think my proposal on naming will achieve what you want as then:
- the variables will be grouped as metrics based on what they are (ie, how the module defines them), with the connection label/id/whatever being available as a label on the metric.
- These labels on metrics are commonly used to filter, or you can query and ignore the label.
- There is no guarantee that a
temperaturevariable will be at all consistent between module types. some will be C some F, some strings, some numbers, so having them grouped by module will both force and allow you to handle that explicitly
For example, a snippet from a grafana graph I have:
That first query returns ~10 sensors of data, the second query matches one or two). The device label is being used to know how to group values within the metric
companion/lib/Service/HttpApi.ts
Outdated
| .replace(/"/g, '\\"') | ||
| .replace(/\n/g, '\\n') | ||
|
|
||
| result += `# HELP ${connectionLabel}:${variableNamePrint} ${labelPrint}\n` |
There was a problem hiding this comment.
Personally, I would be tempted to use https://github.com/siimon/prom-client to build the response of this request, to avoid needing to manually handle this encoding and syntax structure.
I don't know if it can do what we need here though, I have only used it for an application wide endpoint.
companion/lib/Service/HttpApi.ts
Outdated
| message: 'App information retrieved successfully', | ||
| timestamp: getTimestampUnix(), | ||
| data: { | ||
| appinfo: this.#serviceApi.appInfo, |
There was a problem hiding this comment.
I don't like this.
This appInfo object is an internal object that has no guarantees of being at all consistent between versions.
A majority of its contents I can't see as being of any value to a caller.
I am not against exposing some of this data, but I think it needs to be done in a more controlled fashion, with only the useful bits being exposed
companion/lib/Service/HttpApi.ts
Outdated
| * Provides JSON for connection status | ||
| * | ||
| */ | ||
| #connectionGetJson = (_req: Express.Request, res: Express.Response): void => { |
There was a problem hiding this comment.
I don't like how this method works.
Searching for variables like this feels rather brittle and messy.
It would be much cleaner to expose direct access to some methods/events/classes needed to drive this.
I can say that some of this will for sure break a bit with the surface module work which is ongoing, as they are 'instances' too, so will appear in some of these variables
JohannesRegneri
force-pushed
the
http-api
branch
from
a198b50 to
14758cd
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughNew HTTP API endpoints expose variable data in JSON and Prometheus formats. A new method retrieves expression variable definitions through the ControlsController. The prom-client dependency is added for Prometheus metrics support. Documentation is updated across user guide and WebUI settings. Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (2)
companion/lib/Service/HttpApi.ts (2)
688-694: Redundant branches — both do the same thing.The
isExpressionVariablecase (line 691) and theelsecase (line 693) are identical. This looks like a copy-paste oversight. You could simplify this to just anif/elsefor custom vs. everything else.✨ Suggested simplification
- if (isCustomVariable) { - value = this.#serviceApi.getCustomVariableValue(variableName) - } else if (isExpressionVariable) { - value = this.#serviceApi.getConnectionVariableValue(connectionLabel, variableName) - } else { - value = this.#serviceApi.getConnectionVariableValue(connectionLabel, variableName) - } + if (isCustomVariable) { + value = this.#serviceApi.getCustomVariableValue(variableName) + } else { + value = this.#serviceApi.getConnectionVariableValue(connectionLabel, variableName) + }
743-749: Same redundant branches here too.Same simplification opportunity as in the JSON handler — the
isExpressionVariableandelsebranches are identical.✨ Suggested simplification
- if (isCustomVariable) { - value = this.#serviceApi.getCustomVariableValue(variableName) - } else if (isExpressionVariable) { - value = this.#serviceApi.getConnectionVariableValue(connectionLabel, variableName) - } else { - value = this.#serviceApi.getConnectionVariableValue(connectionLabel, variableName) - } + if (isCustomVariable) { + value = this.#serviceApi.getCustomVariableValue(variableName) + } else { + value = this.#serviceApi.getConnectionVariableValue(connectionLabel, variableName) + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
companion/lib/Controls/Controller.tscompanion/lib/Service/HttpApi.tscompanion/lib/Service/ServiceApi.tscompanion/package.jsondocs/user-guide/5_remote-control/http-remote-control.mdwebui/src/UserConfig/Sections/HttpProtocol.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-11T09:42:06.804Z
Learnt from: arikorn
Repo: bitfocus/companion PR: 3849
File: webui/src/Layout/Header.tsx:231-271
Timestamp: 2026-01-11T09:42:06.804Z
Learning: In CoreUI React (coreui/react), prefer using the as prop on CDropdownItem instead of the component prop to render as a different element/component. For TanStack Router integration, use as={Link} (and provide to="/path") to avoid wrapping with CNavLink, e.g. <CDropdownItem as={Link} to="/path">...</CDropdownItem>. This improves compatibility with routing libraries and keeps rendering consistent across layouts. Apply this pattern to TSX files in the webui directory where CoreUI Dropdowns are used.
Applied to files:
webui/src/UserConfig/Sections/HttpProtocol.tsx
🧬 Code graph analysis (2)
companion/lib/Service/ServiceApi.ts (1)
shared-lib/lib/Model/Variables.ts (1)
ModuleVariableDefinitions(12-12)
companion/lib/Service/HttpApi.ts (1)
companion/lib/Registry.ts (1)
Registry(81-484)
🔇 Additional comments (9)
docs/user-guide/5_remote-control/http-remote-control.md (1)
71-103: Great documentation for the new variable endpoints!The documentation is clear, well-structured, and follows the existing format nicely. The Content-Type specifications and Prometheus metric prefix details are particularly helpful for users integrating with these endpoints. Thanks for keeping the docs in sync with the code changes!
companion/lib/Service/ServiceApi.ts (1)
153-159: LGTM!The delegation pattern is clean and consistent with the other variable-related methods in this class (like
getConnectionVariableDefinitionsandgetCustomVariableDefinitions). The JSDoc comment follows the established style. Nice work keeping the API surface consistent!companion/lib/Controls/Controller.ts (1)
403-421: Nice work on this clean implementation!The method is well-structured and follows the existing patterns nicely. The defensive check for a truthy
variableNameon line 412 is a great touch, and the iteration pattern is consistent with similar methods in the class.The type compatibility concern checks out—the return type
Record<string, { name: string; description: string }>perfectly matchesModuleVariableDefinitions(which is defined asRecord<string, VariableDefinition>whereVariableDefinitionis{ name: string; description: string }), so the types are fully compatible with the delegating method in ServiceApi.companion/package.json (1)
83-83: Nice addition of prom-client for Prometheus metrics support!This is the standard library for Prometheus metrics in Node.js applications – great choice! You're using the latest version (15.1.3) with no known security vulnerabilities, so this is a solid, safe addition.
companion/lib/Service/HttpApi.ts (5)
324-328: LGTM! Routes look well-placed.The new endpoints follow the existing naming conventions and are correctly positioned before the catch-all 404 handler. 👍
696-701: Consider whethername: connectionLabelis the intended value.Setting
nameto the connection label for every variable seems a bit unusual. Did you perhaps mean to use the variable's own label (likeobj.label) here instead? IfconnectionLabelis intentional, maybe a clearer property name likeconnectionwould help avoid confusion since you already haveconnection: connectionLabelat the top level of the response.
774-797: Nice implementation of the info-metric pattern for non-numeric values!Using a gauge with value
1and the actual value as a label is the standard Prometheus way to expose string data. The_infosuffix convention is spot-on. 🎯
800-810: Good async error handling.Properly handling the promise from
register.metrics()with both success and error paths. The error logging and 500 response are appropriate.One tiny thought: if you wanted to be extra defensive, you could wrap the entire loop (lines 741-798) in a try-catch as well, in case
new Gauge()throws unexpectedly due to the collision issue mentioned above. But this is optional — what you have is solid!
10-10: Nice choice withprom-client!Great pick using a well-established and actively maintained library for Prometheus metrics. The import is focused and pulls only what's needed for your implementation. Thanks for putting thought into the library selection!
companion/lib/Service/HttpApi.ts
Outdated
| // Create sanitized metric name | ||
| const sanitizedVariableName = variableName.replace(/[^a-zA-Z0-9_]/g, '_') | ||
| const metricName = isCustomVariable | ||
| ? `companion_custom_variable_${sanitizedVariableName}` | ||
| : isExpressionVariable | ||
| ? `companion_expression_variable_${sanitizedVariableName}` | ||
| : `companion_connection_variable_${sanitizedVariableName}` |
There was a problem hiding this comment.
Minor heads-up: metric name collisions are theoretically possible.
The sanitization variableName.replace(/[^a-zA-Z0-9_]/g, '_') could map different variable names to the same metric name (e.g., foo-bar and foo.bar both become foo_bar). This would cause prom-client to throw an error about duplicate metric registration.
In practice this is probably rare, but if you want to be extra safe, you could catch and log metric registration errors within the loop rather than failing the whole request.
| <li> | ||
| Get all custom variables as Prometheus metrics | ||
| <br /> | ||
| Method: <code>GET</code> | ||
| <br /> | ||
| Path: <code>/api/variables/custom/prometheus</code> | ||
| </li> | ||
| <li> | ||
| Get all expression variables as Prometheus metrics | ||
| <br /> | ||
| Method: <code>GET</code> | ||
| <br /> | ||
| Path: <code>/api/variables/expression/prometheus</code> | ||
| </li> | ||
| <li> | ||
| Get all module/connection variables as Prometheus metrics | ||
| <br /> | ||
| Method: <code>GET</code> | ||
| <br /> | ||
| Path: <code>/api/variables/</code><Connection label><code>/prometheus</code> | ||
| </li> | ||
| Press page 1 row 0 column 2 | ||
| <br /> | ||
| POST <code>/api/location/1/0/2/press</code> | ||
| </p> | ||
|
|
||
| </ul> |
There was a problem hiding this comment.
Small structural issue and a consistency suggestion
A couple of things I noticed:
-
Structural issue: Lines 183-185 ("Press page 1 row 0 column 2...") appear to be an example that ended up outside of a
<li>element and inside the<ul>. This looks like it might be leftover from the previous structure. -
Consistency: The JSON endpoints include
Content-Type: <code>application/json</code>, but the Prometheus endpoints don't mention their content type (text/plain; version=0.0.4). The markdown docs include this info – might be nice to have it here too for consistency!
Suggested fix for the structural issue
<li>
Get all module/connection variables as Prometheus metrics
<br />
Method: <code>GET</code>
<br />
Path: <code>/api/variables/</code><Connection label><code>/prometheus</code>
</li>
- Press page 1 row 0 column 2
- <br />
- POST <code>/api/location/1/0/2/press</code>
</ul>
+ <p>
+ <strong>Examples:</strong>
+ </p>
+ <p>
+ Press page 1 row 0 column 2
+ <br />
+ POST <code>/api/location/1/0/2/press</code>
+ </p>4 participants
…and prometheus metrics #2424
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.