[confmap] Remove top level lock when considering struct as Unmarshalers. #9862
Conversation
atoulme
changed the title
atoulme
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9862 +/- ##
==========================================
- Coverage 91.54% 91.53% -0.01%
==========================================
Files 360 360
Lines 16695 16699 +4
==========================================
+ Hits 15284 15286 +2
- Misses 1074 1075 +1
- Partials 337 338 +1 ☔ View full report in Codecov by Sentry. |
|
It would also be interesting to try and reproduce the failing contrib test on a unit test here, so we can work on it. The offending |
**Description:** Adds two tests to confmap to test some edge cases **Link to tracking Issue:** Written while reviewing #9862
added a skipped test here: TestRecursiveUnmarshaling |
atoulme
force-pushed
the
remove_top_level_block
branch
from
8637f2f
to
5ad613b
Compare
|
If you want a foolproof solution, we might have to do something a little more drastic. We might need to keep a hashset of all pointers of unmarshaler methods ever called, to make sure we don't run into cycles. |
I guess we can forbid what the stanza package does instead. Do you think there is a benefit to being able to do what stanza does? I can't see any advantage right now |
|
No, I don't see any advantage, except maybe if you're trying to unmarshal and set all values on the struct atomically. Even then, this is a corner case. I think we're ok. |
There was a problem hiding this comment.
I am okay with the current approach, I guess we should document the limitation about nested unmarshalling before merging, but I think we can go with this.
cc @open-telemetry/collector-approvers, I would appreciate a second pair of eyes since this is somewhat subtle. Even if this is ready before, I will wait at least until Monday to merge to give others some time to review.
|
One more PR in contrib needed: open-telemetry/opentelemetry-collector-contrib#32577 |
**Description:** Update contrib to match core with the latest changes that are needed for open-telemetry/opentelemetry-collector#9862
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
atoulme
force-pushed
the
remove_top_level_block
branch
from
a5083b2
to
9afa5ef
Compare
|
The last contrib issue is solved by open-telemetry/opentelemetry-collector-contrib#32660. Unfortunately, I couldn't find a good way to fix this - in effect, a breaking change. |
| @@ -41,6 +41,10 @@ func NewFromStringMap(data map[string]any) *Conf { | |||
| // The confmap.Conf can be unmarshalled into the Collector's config using the "service" package. | |||
| type Conf struct { | |||
| k *koanf.Koanf | |||
| // If true, upon unmarshaling do not call the Unmarshal function on the struct | |||
| // if it implements Unmarshaler and is the top-level struct. | |||
| // This avoids running into a circular dependency. | |||
There was a problem hiding this comment.
We should define what the circular dependency is here. I think calling out the potential for an infinite recursion caused by Unmarshaler.Unmarshal and Conf.Unmarshal calling each other would probably be a good mention.
This is a slice of #9750 focusing on removing the top level lock on unmarshaling structs.