Skip to content

fix(env): use mpsc for autodiscovery events#1671

Open
omribz156 wants to merge 2 commits into
DataDog:mainfrom
omribz156:codex/autodiscovery-mpsc
Open

fix(env): use mpsc for autodiscovery events#1671
omribz156 wants to merge 2 commits into
DataDog:mainfrom
omribz156:codex/autodiscovery-mpsc

Conversation

@omribz156
Copy link
Copy Markdown

Summary

Changes autodiscovery event delivery from a bounded broadcast channel to a bounded MPSC channel. Sends are now awaited, so a burst of autodiscovery events applies backpressure instead of letting a lagging broadcast receiver miss events.

This also updates the provider receiver type, hands out the single receiver through the existing subscribe() API, and adds a local-provider regression test that uses a capacity-1 channel with two config events.

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

How did you test this PR?

  • cargo fmt --check
  • git diff --check
  • cargo metadata --no-deps --format-version 1

I attempted cargo test -p saluki-env autodiscovery::providers::local --lib, but this Windows workspace is missing the MSVC linker / Windows SDK libraries (link.exe, kernel32.lib, etc.), so the command fails while compiling dependency build scripts before it reaches Saluki code.

This was implemented with Codex assistance, with the final diff reviewed against the issue scope and the existing autodiscovery provider tests.

References

@omribz156 omribz156 requested a review from a team as a code owner May 18, 2026 06:06
@dd-octo-sts dd-octo-sts Bot added the area/config Configuration. label May 18, 2026
Copy link
Copy Markdown
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

While this PR is superficially correct, the implementation misses an important aspect: only one subscriber can exist now, and only one ever, not even just "at most one."

What we need to do instead is actually keep track of the separate subscriptions, creating an MPSC channel for every subscription and holding on to the senders. Every time we go to send, we'd use the result of the send -- did it succeed or is the receiver gone? -- and use that to influence whether or not we remove the attached sender from the list before forwarding the next event.

@omribz156
Copy link
Copy Markdown
Author

Thanks, fixed in the latest push.

Changed both autodiscovery providers to keep a list of per-subscriber mpsc::Senders. Each subscribe() now creates its own bounded channel, and event delivery fans out to all current subscribers while pruning senders whose receivers have gone away. I also added local-provider coverage for fan-out plus closed-receiver pruning.

Verified with:

  • cargo fmt --check
  • cargo metadata --no-deps --format-version 1
  • git diff --check

I also tried cargo test -p saluki-env autodiscovery::providers::local --lib, but this Windows environment fails earlier in process-memory with cannot find type StatSource in this scope, before the focused autodiscovery tests can run.

This follow-up was Codex-assisted, with the final diff reviewed before pushing.

@datadog-datadog-prod-us1-2
Copy link
Copy Markdown

Pipelines

Fix all issues with BitsAI

⚠️ Warnings

🚦 1 Pipeline job failed

DataDog/saluki | check-fmt   View in Datadog   GitLab

🔧 Fix in code (Fix with Cursor). Formatting issues in autodiscovery.rs: unexpected use of 'use std::sync::Arc' which is already imported.

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 1e7bf59 | Docs | Datadog PR Page | Give us feedback!

@tobz
Copy link
Copy Markdown
Member

tobz commented May 22, 2026

Looks like all that's left is fixing the formatting lint and then this should be good to go.

gh-worker-dd-mergequeue-cf854d Bot pushed a commit that referenced this pull request May 22, 2026
…ns (#1723)

## Summary

As stated in the PR title.

PRs from external contributors (as in forks) do not trigger CI: only branches pushed directly to the repository (and thus, implied by that, authorized contributors) can trigger CI.

In order to make it possible to accept external contributions and, in doing so, verify that they pass CI, we need a way to repeatably trigger CI. This PR adds a new Claude skill that automates the manual steps of doing so with a command invocation that's as simple as:

`/external-pr-ci-trigger <PR number>`

This skill uses the `gh` CLI, and normal Git commands, to pull down the remote fork/branch and repush them to the main repository with a specific branch naming scheme. The push alone triggers CI, but the branch name matching a specific format will allow the CI pipeline to be automatically associated with the external contributor's PR, so that all check status is propagated just as if it was triggered directly.

## Change Type

- [ ] Bug fix
- [x] New feature
- [ ] Non-functional (chore, refactoring, docs)
- [ ] Performance

## How did you test this PR?

Ran the skill locally on #1671 and observed that it created the branch _and_ triggered CI properly, with the PR showing the checks running.

## References

DADP-2


Co-authored-by: toby.lawrence <toby.lawrence@datadoghq.com>
dd-octo-sts Bot pushed a commit that referenced this pull request May 22, 2026
…ns (#1723)

## Summary

As stated in the PR title.

PRs from external contributors (as in forks) do not trigger CI: only branches pushed directly to the repository (and thus, implied by that, authorized contributors) can trigger CI.

In order to make it possible to accept external contributions and, in doing so, verify that they pass CI, we need a way to repeatably trigger CI. This PR adds a new Claude skill that automates the manual steps of doing so with a command invocation that's as simple as:

`/external-pr-ci-trigger <PR number>`

This skill uses the `gh` CLI, and normal Git commands, to pull down the remote fork/branch and repush them to the main repository with a specific branch naming scheme. The push alone triggers CI, but the branch name matching a specific format will allow the CI pipeline to be automatically associated with the external contributor's PR, so that all check status is propagated just as if it was triggered directly.

## Change Type

- [ ] Bug fix
- [x] New feature
- [ ] Non-functional (chore, refactoring, docs)
- [ ] Performance

## How did you test this PR?

Ran the skill locally on #1671 and observed that it created the branch _and_ triggered CI properly, with the PR showing the checks running.

## References

DADP-2

Co-authored-by: toby.lawrence <toby.lawrence@datadoghq.com> 09880b1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Configuration.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change autodiscovery channel to MPSC to avoid loss.

2 participants