Improve OpenTelemetry Collector JSON Marshalling

[iblancasa]

Fixes #3281

[swiatekm]

I don't understand what is actually going on in this PR. From the tests, it seems like everything works correctly, but I don't know why. I've left some comments asking questions, but in general I think you should exhaustively explain the weird marshaling functions in docstrings.

[iblancasa]

I don't understand what is actually going on in this PR. From the tests, it seems like everything works correctly, but I don't know why.

I tried to explain it in the changelog:

Previously, the v1beta1 OpenTelemetryCollector CR's config field was incorrectly
marshalled, potentially causing issues when storing in etcd or retrieving via kubectl.
This fix ensures the config is properly marshaled, maintaining the correct
structure and data types. For example, a config with nested maps and arrays will now
be correctly preserved when stored and retrieved, instead of being flattened or
losing structure.

The problem happens when you want to create one OpenTelemetry Collector instance like this one:

apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
metadata:
  name: otel
spec:
    config:
        receivers:
            otlp:
                http:
                grpc:

Then, you create it in the cluster with, for instance, kubectl.

kubectl apply -f otelcol.yaml

When you get the OpenTelemetry Collector instance, you will get this:

$ kubectl get otelcol -o yaml otel
apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
metadata:
  name: otel
spec:
    config:
        receivers:
            otlp:

We have a warning mentioning this can happen, but it can be difficult for users to see.

I'll add more info to the changelog.

I've left some comments asking questions, but in general I think you should exhaustively explain the weird marshaling functions in docstrings.

I just added some comments to the source code.

[jaronoff97]

I'm still not entirely sure how or why this works... maybe we can talk about it in slack?

[jaronoff97]

why do we only need to do this for receivers?

[jaronoff97]

@iblancasa bump on this

[iblancasa]

I'm still not entirely sure how or why this works... maybe we can talk about it in slack?

Just messaged you.

[iblancasa]

The E2E tests are failing because there was a problem while downloading a dependency or something.

[iblancasa]

@jaronoff97 @swiatekm do you have any other questions or concerns regarding this PR?

[jaronoff97]

@iblancasa bump on this

[jaronoff97]

ive found this fake client package a lot more usable

[jaronoff97]

OOC why are the hashes changing? id be interested to understand how the full content is being changed.

[jaronoff97]

@swiatekm reference https://medium.com/picus-security-engineering/custom-json-marshaller-in-go-and-common-pitfalls-c43fa774db05

[iblancasa]

Closing this PR because the changes introduced in #3333 workaround the problem.

Since it seems it was a problem for so many people to follow what's going on this PR, I'll add an explanation here just as a reference.

The issue

After creating an OpenTelemetryCollector instance from a YAML file like this one:

apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
metadata:
  name: otel
spec:
    config:
        receivers:
            otlp:
                protocols:
                    http:
                    grpc:

If you get the OpenTelemetryCollector instance from Kubernetes:

kubectl get otelcol -o yaml otel

this is what you get:

apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
metadata:
  name: otel
spec:
    config:
        receivers:
            otlp:
                protocols:

The values for protocols are missing. This does not just apply to otlp receiver but to all those configurations with a key but not a value -like what happens for grpc or http in the previous example.

This is not supported by other tools like kustomize but, if you are an OpenTelemetry Collector user, you would expect to be able to use the same kind of config that you would use with the regular Go binary.

The operator shows a warning in this situation.

The fix

The fix is based on this blog article.

I was implementing the MarshalJSON for the OpenTelemetry, OpenTelemetrySpec and Config structs to ensure the custom MarshalJSON method from AnyConfig would be called -because it seems in some scenarios the custom MarshalJSON implementations are not called when one struct is part of another, I don't get why-. I was forcing the call to the custom MarshalJSON only for receivers because are the only kind of components where this situation can be reproduced. Why? Because receiver components are the only ones that contain configurations where keys without values are valid.

I added a unit test to ensure this situation, which seems to be reproducible only when the object is created + retrieved in Kubernetes, didn't reproduce again.

The unexpected workaround

With #3333, all those keys that would not have any value, have now a value by default. So, the issue is there but users will not suffer it anymore.

[swiatekm]

@iblancasa I think I finally understand, thanks for being so patient with us! Could we actually keep the tests you've written, and skip them for the time being? We've been talking about alternative solutions to this issue, and the tests would be very helpful as benchmarks. I'd also hate for your hard work to go to waste this way.