Skip to content

Behaviour of ToStringMap Not Matching Confmap Decoding Workflow #11749

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

Closed
mahadzaryab1 opened this issue Nov 26, 2024 · 6 comments · Fixed by #12872
Closed

Behaviour of ToStringMap Not Matching Confmap Decoding Workflow #11749

mahadzaryab1 opened this issue Nov 26, 2024 · 6 comments · Fixed by #12872

Comments

@mahadzaryab1
Copy link
Contributor

This question is related to #11734 which is working towards #10260 and #10266. The PR I have open enables the decoding of nil values in mapstructure so that we can use an Optional type with a default value (see #10260 (comment) for more context). While enabling the decoding of nil values, I came across an issue where we were unable to differentiate between nil and empty slices. The reason for that is because we always initialize a nil slice and do not populate it if it is empty (see https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/confmap.go#L135). In order to get around this, I added this patch in the PR above.

		if m == nil {
			return newSlice
		}
		newSlice = []any{}

This allows us to differentiate between a nil slice and an empty slice which we need in the zeroSliceHookFunc. However, that patch causes this unit test to fail because there's an expectation here that an empty slice gets rendered as nil when calling ToStringMap. I wanted to ask if it was valid to change this expectation so that it matches the decoding workflow we have in which an empty slice is preserved. cc @yurishkuro

@mx-psi
Copy link
Member

mx-psi commented Nov 26, 2024

I would be okay changing this (I consider this an implementation detail). I believe this test was added by @songy23 because a change broke some tests on the Datadog Agent, but so long as it is clearly listed in the changelog and it does not break existing use cases (Datadog or otherwise) I am okay with it.

@mahadzaryab1
Copy link
Contributor Author

@mx-psi Thanks for getting back to me! Would you prefer I land the patch in a dedicated PR or is it fine for it to go in #11734?

@mx-psi
Copy link
Member

mx-psi commented Nov 26, 2024

I would prefer a dedicated PR for this :)

@mahadzaryab1
Copy link
Contributor Author

@mx-psi I've got a PR up at #11755

@yurishkuro
Copy link
Member

yurishkuro commented Mar 11, 2025

Addressed in #11882 and #12872

@mx-psi
Copy link
Member

mx-psi commented Apr 16, 2025

@yurishkuro I am trying to do this again, this time hopefully without breaking anything. See #12872

github-merge-queue bot pushed a commit that referenced this issue Apr 21, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

A second attempt at #11882.

Note that I added some tests in #12871 to prevent something like #12661
from happening again.

#### Link to tracking issue
Fixes #11749

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->

See tests from #12871 as well as the new tests added here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants