-
Notifications
You must be signed in to change notification settings - Fork 490
fix(internal/remoteconfig): allow Remote Config singleton to be reentrant #4118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(internal/remoteconfig): allow Remote Config singleton to be reentrant #4118
Conversation
BenchmarksBenchmark execution time: 2025-11-20 11:39:02 Comparing candidate commit 9cf232b in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 0 unstable metrics. |
felixge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave this a light review, but mostly LGTM.
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 9cf232b | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a51499e to
e85adca
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
kakkoyun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
I believe we can simplify by using a mutex and regular boolean and just closing the client.stop once.
e85adca to
fa80a57
Compare
kakkoyun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to guard the Reset, I believe.
One other thing we can explore it to have an atomic.Pointer for the global client.
And embed the started logic into the Client since it has already have its own mutex. We can embed the logic in Start() into Client.start() and Start can proxy to it.
We can swap the global client after a successful start.
WDYT?
I tested this and it introduced too many changes. I think we need to find a better way to handle multiple products that run when the tracer is started. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What does this PR do?
Models
internal/remoteconfigpackage's behaviour to mirror howSetGlobalTracerbehaves when callingStart()multiple times.AI generated PR.Motivation
Fixes #4111.
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!