Skip to content

Conversation

@goxberry
Copy link
Contributor

What does this PR do?

This pull request adds types needed to be able to expose observer configuration to users via lading.yaml files in later work. It does not change the semantics of lading.yaml -- rather, this code provides enough setup to make clear a proposed observer configuration UI.

Motivation

The eventual idea is to make the observer optional and have users enable it via adding

observer: linux

to lading.yaml.

Related issues

SMPTNG-721, SMPTNG-601.

Additional Notes

Exposing observer configuration via a new field with variants seems more consistent with the design of Lading than, e.g., using a Boolean-valued field. Using an enum-valued field also enables future extension of the observer component to additional variants, if desired.

I think the quickest way to make an observer optional in Lading is to
add an enumerated type specifying the variants of observer that are
available. While the Linux observer is currently the only observer
variant available, it may be desirable to add others in the
future. Compared to a boolean option, using an enumerated type more
easily enables later extension of the observer concept.

Signed-off-by: Geoffrey M. Oxberry <geoffrey.oxberry@datadoghq.com>
I'd eventually like to make the observer an optional part of Lading
enabled by adding `observer: linux` to `lading.yaml` files. In order
to do that, I need to add some types to define a Linux observer
variant and to store its variant-specific configuration. This commit
adds those types.

The `core::Default` implementations written in this commit are
intended to be temporary to preserve the existing `lading.yaml`
semantics that treat the observer as mandatory when `lading` is run
with a target.

Signed-off-by: Geoffrey M. Oxberry <geoffrey.oxberry@datadoghq.com>
@goxberry goxberry requested a review from a team as a code owner November 14, 2025 05:02
Copy link
Collaborator

@blt blt left a comment

Choose a reason for hiding this comment

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

One thing we've avoided so far is making lading.yaml configs OS specific. I'd like to keep that up. How does it strike you to have something like:

observer: {}

in the config? If present, observer is on, if not present observer is off. Or, I guess, to avoid needing to update every lading.yaml config we need to preserve the existing default-on behavior, so maybe something like:

observer:
    enabled: false

I wouldn't mind, also, dropping the use of serde flatten. I've come to feel over time this is a grave mistake.

@goxberry
Copy link
Contributor Author

One thing we've avoided so far is making lading.yaml configs OS specific. I'd like to keep that up.

Instead of calling the existing observer Linux, would something OS-neutral like Native work?

How does it strike you to have something like:

observer: {}

in the config? If present, observer is on, if not present observer is off.

The syntax seems...counterintuitive?

I think preserving compatibility suggests that omitting an observer key in a config file would enable that component, whereas omitting keys for other components disables them. That inversion seems like a potential surprise to users. However, I don't like the idea of having to change every lading.yaml file, and if we pursued that change, I imagine we would need to write a tool that checks captures for field(s) we would expect to be present with an observer enabled.

Or, I guess, to avoid needing to update every lading.yaml config we need to preserve the
existing default-on behavior, so maybe something like:

observer:
   enabled: false

I lean towards that option for the time being, even though I think these semantics potentially cause a backwards compatibility problem later if we explore some of the ideas @GeorgeHahn has suggested. I could also see replacing enabled with an OS-neutral name for the existing observer (e.g., native: false).

I wouldn't mind, also, dropping the use of serde flatten. I've come to feel over time this is a grave mistake.

I'm curious why you think use of #[serde(flatten)] has been a grave mistake. I'm not particularly attached to using #[serde(flatten)] in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants