Set library version with SDK language #288

bryfox · 2025-03-14T12:41:14Z

Changelog

The SDK language is now reflected in the library written to MCAP files, and is included in serverInfo metadata under the key fg-library.

Description

Example: foxglove-sdk-python/v0.3.0.

The language name is set at build time, based on environment. For C++, which is a wrapper around the C library, a function is called by the log sinks when they're created to override that language.

The app may use the serverInfo metadata for troubleshooting or performance analysis. Foxglove has some precedent for the "fg-" prefix in user agents and HTTP header keys. "fg-library" echoes the "library" header of mcap.

linear · 2025-03-14T12:41:17Z

FG-10630 Annotate mcaps + websocket connections with library + version

jtbandes · 2025-03-14T18:30:20Z

.github/workflows/c_cpp.yml

      - name: Build C library for ${{ matrix.target }}
+        env:
+          FOXGLOVE_SDK_LANGUAGE: c
        run: cargo build --release
        working-directory: c
      - name: Ensure generated files are up to date
        run: git diff --exit-code

      - name: Build C++ library and run tests
        if: ${{ !fromJson(matrix.cross) }}
+        env:
+          FOXGLOVE_SDK_LANGUAGE: cpp
        run: make test
        working-directory: cpp


I don't think this is going to work – the C++ sdk is just a wrapper for better ergonomics of calling the C sdk. The user will link the C sdk with their program and use the wrappers to call functions from it. So there is not really a "separate" build of the two SDKs (we aren't currently planning to ship a prebuilt C++ library, only C)

As for alternatives, some ideas:

If this is used in specific places (like websocket server & mcap writer), the c++ sdk wrappers could pass in some value when creating those to override the defaults (but we'd have to remember to do this in every relevant location)

We could add a global function to set the library name at runtime (though would have to decide when to call it? might end up being pretty horrendous to do this in a cross-platform way, unless we want to make the users just call some init function explicitly)

I really hope someone else can come up with a better 3rd option 😄

Yeah, I completely missed that this step doesn't run cargo build. Runtime it is.

I thought about something similar to the second idea; in python, it seems reasonable for us to call a function during the module initialization. I'll see if I can find something that seems workable across the board.

I'm leaning towards a hybrid approach: introduce a compiled_sdk_language build-time environment variable to replace what's here, and then have a static init step in c++ which can override this. Then at runtime, build a string with the compiled version. That sidesteps the cross-platform issue you linked, and I'd like to avoid providing something in the API to set the library name / language directly.

When I tried this out, I get a cmake error — something internal to Catch2 — unless I skip building the tests. I'll have to solve whatever issue that is (assuming it's not uncovering a legitimate problem), but would like your thoughts on the approach before spending more time on it.

I guess I'm not sure what you mean by a static init step, you mean just forcing the user to call something like foxglove::initSDK()? I guess that's fine. We could also call it ourselves each time a Channel is constructed, as an example

also, for the Catch2 issue, maybe try rebasing - I fixed something related to that recently but it was a linker error, not a cmake error, so maybe different from what you ran into

Another alternative is that we could simply accept that we only have one string which is used for both C/C++ SDKs. Then assume probably 99%+ of users would be using it via C++. I guess it depends exactly what problem we are trying to solve here

Thanks. I'm not sure what the cmake error was, but it eventually went away after commenting/uncommenting things in the file (maybe something cached?). Initially I was thinking I could statically initialize something inside an unnamed namespace to take care of this, but I think that was misguided.

I updated to make the calls in the writer & server constructors.

simply accept that we only have one string which is used for both C/C++

This is somewhat user-facing in that it ends up as the 'library' of mcap files, so I think picking either one of those could seem strange. Maybe there's some other name we can use to suggest 'either', but I think I prefer dealing with it at runtime.

The main drawback to me is having this C function around that doesn't have any use for end users.

c++ requires runtime changes

jtbandes · 2025-03-21T22:32:54Z

c/src/lib.rs

@@ -441,6 +441,12 @@ pub unsafe extern "C" fn foxglove_channel_log(
    );
 }

+/// For use by the C++ SDK. Identifies that wrapper as the source of logs.
+#[unsafe(no_mangle)]
+pub extern "C" fn _register_cpp_wrapper() {


I think the leading underscore is technically not allowed in C/C++ https://devblogs.microsoft.com/oldnewthing/20230109-00/?p=107685 (Maybe trailing underscore is ok? or use "internal" in the name?)

I also would recommend including "foxglove" somewhere in the name

jtbandes · 2025-03-21T22:33:36Z

.github/workflows/c_cpp.yml

+        env:
+          FOXGLOVE_SDK_LANGUAGE: cpp


maybe this can be removed now?

jtbandes · 2025-03-21T22:36:15Z

rust/foxglove/src/library_version.rs

+//! Provides an identifier for the library used as a log source.
+use std::sync::OnceLock;
+
+use crate::SDK_LANGUAGE;


nit: I find it a bit confusing that these two things are defined in different files, but they seem quite related

jtbandes · 2025-03-21T22:36:53Z

rust/foxglove/src/lib.rs

@@ -194,6 +198,12 @@ pub use sink::{Sink, SinkId};
 pub(crate) use time::nanoseconds_since_epoch;
 pub use websocket_server::{WebSocketServer, WebSocketServerBlockingHandle, WebSocketServerHandle};

+pub(crate) static SDK_LANGUAGE: LazyLock<String> = LazyLock::new(|| {


consider naming this with "build"/"compile" somewhere in the name to indicate it was set at compile time?

Does this need to be LazyLock? It looks like it is read-only after creation

I updated the name. LazyLock (or something like it) is needed because the unwrapping happens at runtime. I didn't see a way to define a default env var for the build, so would like to keep it optional.

jtbandes · 2025-03-21T22:39:07Z

rust/foxglove/src/library_version.rs

+
+/// Returns an identifer for this library, for use in log sinks.
+pub(crate) fn get_library_version() -> String {
+    let language = CELL.get_or_init(|| SDK_LANGUAGE.as_str());


So I guess this means if get_library_version is called before set_sdk_language, then the latter will have no effect? That's probably fine, but wonder if it should be called out in the comments

…ections-with-library-version

bryfox requested review from jtbandes, eloff, amacneil and gasmith March 14, 2025 13:05

gasmith previously approved these changes Mar 14, 2025

View reviewed changes

jtbandes reviewed Mar 14, 2025

View reviewed changes

bryfox added 2 commits March 21, 2025 14:52

Set library version in builds

81a0736

Set language at runtime for c++

779bf58

bryfox force-pushed the bryan/fg-10630-annotate-mcaps-websocket-connections-with-library-version branch from aa45717 to 779bf58 Compare March 21, 2025 19:05

Update header

c650c92

bryfox requested a review from gasmith March 21, 2025 20:38

jtbandes approved these changes Mar 21, 2025

View reviewed changes

bryfox added 2 commits April 3, 2025 11:18

Address feedback

fcebc5f

Merge branch 'main' into bryan/fg-10630-annotate-mcaps-websocket-conn…

0dc1544

…ections-with-library-version

bryfox merged commit 428dc2c into main Apr 3, 2025
38 checks passed

bryfox deleted the bryan/fg-10630-annotate-mcaps-websocket-connections-with-library-version branch April 3, 2025 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set library version with SDK language #288

Set library version with SDK language #288

Uh oh!

bryfox commented Mar 14, 2025 •

edited

Loading

Uh oh!

linear bot commented Mar 14, 2025

Uh oh!

jtbandes Mar 14, 2025

Uh oh!

jtbandes Mar 14, 2025 •

edited

Loading

Uh oh!

bryfox Mar 14, 2025

Uh oh!

bryfox Mar 18, 2025

Uh oh!

jtbandes Mar 21, 2025 •

edited

Loading

Uh oh!

jtbandes Mar 21, 2025

Uh oh!

bryfox Mar 21, 2025

Uh oh!

jtbandes Mar 21, 2025

Uh oh!

jtbandes Mar 21, 2025

Uh oh!

jtbandes Mar 21, 2025

Uh oh!

jtbandes Mar 21, 2025

Uh oh!

bryfox Apr 3, 2025

Uh oh!

jtbandes Mar 21, 2025

Uh oh!

Uh oh!

Uh oh!

Set library version with SDK language #288

Set library version with SDK language #288

Uh oh!

Conversation

bryfox commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Description

Uh oh!

linear bot commented Mar 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtbandes Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtbandes Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bryfox commented Mar 14, 2025 •

edited

Loading

jtbandes Mar 14, 2025 •

edited

Loading

jtbandes Mar 21, 2025 •

edited

Loading