Skip to content
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

doc: add detail to confmap watcher #12150

Merged
merged 4 commits into from
Jan 28, 2025
Merged

Conversation

mattsains
Copy link
Contributor

Description

I tried to improve the docs on watchers based on the issue below

Link to tracking issue

Fixes #12115

Documentation

Clarified some code comments that were incorrect (there's no Retrieved.Get method) and included a new code sample for a Provider that uses the watcher func

@mattsains mattsains requested review from mx-psi, evan-bradley and a team as code owners January 22, 2025 00:08
@mattsains mattsains force-pushed the watcher-docs branch 2 times, most recently from ba832d9 to f833595 Compare January 22, 2025 00:10
@mattsains
Copy link
Contributor Author

I can write a CHANGELOG entry for this, but I don't think it's helpful. Let me know if I should anyway to get the CI to pass

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.84%. Comparing base (b09a65b) to head (cf2d8cf).
Report is 44 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12150      +/-   ##
==========================================
+ Coverage   91.73%   91.84%   +0.11%     
==========================================
  Files         463      465       +2     
  Lines       24819    25325     +506     
==========================================
+ Hits        22767    23261     +494     
- Misses       1670     1675       +5     
- Partials      382      389       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mx-psi mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 22, 2025
confmap/README.md Outdated Show resolved Hide resolved
confmap/provider_test.go Outdated Show resolved Hide resolved
confmap/provider_test.go Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Jan 23, 2025

@bogdandrutu Could you double check this since you designed this in the first place?

Comment on lines 77 to 78
// sleep for 1.5 seconds (1s for a new configuration, and 0.5 more to prevent races. Too close to 2s and we risk getting notified twice)
time.Sleep(1500 * time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this, I think this introduces the risk of flakes on CI, what about

  1. defining a channel ack := make(chan struct{})
  2. On watcherFunc, sending a struct{}{} to ack
  3. Replacing this sleep with a wait on ack

We can probably replace the second sleep with a similar pattern.

What do you think?

Copy link
Contributor Author

@mattsains mattsains Jan 27, 2025

Choose a reason for hiding this comment

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

I replaced the first sleep with a channel (the one waiting for a notification of a new config). However, keeping in mind that this is destined to be documentation and not test case, I couldn't find a natural way to notify when the retriever has unsubscribed the watchFunc. Any way I tried to do it compromised the structure of the Provider in a way that I think would detract from the documentational value. For example:

  • I considered returning a channel as the rawConf return, but that doesn't make sense because the config is never going to be a channel in normal uses.
  • I considered cancelling the context of the Retrieve() method call, but that doesn't make sense because you usually wouldn't do that in a Provider.
  • I also considered creating a channel as a public instance member of the Provider, but again that is not didactically sound because a single Retrieve call has a different duration/lifetime than the entire Provider instance.

I do agree that the sleeps introduce a chance of a race in the tests. I've removed the test for the unsubscribe of the watcher. If you'd still like that tested, I'm just not sure how to do that without making it more like a confusing test and less like documentation

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for taking a look at this, and sorry that this slipped on the first review. I think this looks good to merge now, if we get feedback that this is confusing or the 'unsubscribe watcher' would be helpful, we can add it.

@mx-psi mx-psi added this pull request to the merge queue Jan 28, 2025
Merged via the queue into open-telemetry:main with commit 96ba827 Jan 28, 2025
53 checks passed
@mattsains mattsains deleted the watcher-docs branch January 28, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation and example for confmap/Provider interface which actually has a callback mechanism
3 participants