[Feature Request] Adding Acceptance tests for OTel workflows

[blewis12]

Component(s)

No response

Is your feature request related to a problem? Please describe.

There have been multiple instances of regressions related to unexpected metric names being emitted by SDK’s and/or received by the collector that were not caught during testing. Some, like 12918, have already been addressed by implementing further tests. But there are still cases where this is causing issues. For example, a recent collector version bump caused the collector to reject incoming metrics from the .NET SDK (description of issue here). At Grafana, this led to an internal incident. Although test suites exist, scenarios like this are quite hard to capture without full end to end testing.

Despite an incident occurring related to the backwards incompatible change linked above, we were able to understand the root cause with the help of an internally developed (but OSS) integration testing framework called OATs (OpenTelemetry Acceptance Tests). OATs has worked quite well for us to be able to check for any compatibility issues related to OTel libraries, within the context of the LGTM stack and OTel collector (e.g. for .NET).

Describe the solution you'd like

It would be useful to have a blackbox testing framework for end-to-end (application -> collector -> backend) testing. Although the OTel ecosystem does not include the database level in its scope, the framework should ensure that the collector is sending out valid data to the eventual backend storage. This framework could validate the contract for each of the steps that signals must travel through to land in the backend storage. This test setup would also make requests over a network so that we are testing the validity of information sent over the wire between each step.

If it makes sense, we would be happy to offer OATs to do the job - or to shape OATs into a tool that would be beneficial for the community - under the OTel umbrella.

Describe alternatives you've considered

So far we have not been able to find a framework or test suite within the OTel ecosystem that could help proactively detect version incompatibilities such as those described above. However, if something like this already exists, please do let us know.

Additional context

• Recent sig call where this was discussed
• Recent slack thread
• This issue, though more specifically related to the .NET SDK, was also brought up here

Tip

_{React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}

[bogdandrutu]

I am 100% supporting of this effort, but please make sure we add only tests for things that respect the contract, for example in case of #13727 the source (as mentioned there) does not respect the contact.

Another big problem with this is flakiness, we should make sure these are not flaky (I've seen this in the past) because if they are people will ignore them all.

[blewis12]

Hi @bogdandrutu - thanks for the feedback! I'm not sure I understand what you mean by #13727 not respecting the contract, do you mind explaining that?

Regarding flakiness as well, that's a good point - I haven't heard cases of flakiness within our OATS setup, but @zeitlinger or @martincostello perhaps you could speak to that concern with your experience

[dmathieu]

While I'm also 100% supporting this effort, I do also wonder if it's the proper fix for this specific issue.
If the problem does indeed come from the .NET SDK, this shouldn't be a test within the collector, but within that SDK.
We can't have integration tests within the collector for every language that has an SDK.

Should there be an initiative to add acceptance tests to every SDK? (this would be a specification thing then).

[zeitlinger]

Regarding flakiness

This hasn't been a problem so far, because the tests run in a loop until the telemetry data is found (or timed out).

[zeitlinger]

If the problem does indeed come from the .NET SDK, this shouldn't be a test within the collector, but within that SDK.

A blackbox test can't tell where the problem is - only prevent an accidental regression.
What or where to address the problem is another question.

I think it would be good to have such a test on both ends, the SDKs and the Collector.

To quote @martincostello from slack:

Our understanding is that the .NET SDK is still compliant with the protocol. I don’t have immediate access to the spec to go find and copy-paste the specific sentence, but it’s following something it’s permitted to do. It’s just not the specific bit of the spec that’s already been cited in the original issue by you.
An issue for adding end-to-end tests to the .NET SDK specifically is already linked to in the proposal that was just shared, but if the SDK is compliant as we believe it is, it still wouldn’t necessarily have found an issue either.
The idea being that end-to-end tests flush out things people haven’t thought of and/or are caused by issues across repositories. It’s an extra level of validation to catch impactful issues like the one described, regardless of whose "fault" it is (or is perceived to be).
The aim is to not break end-users, and when I found the original issue, it was as an end-user (not a Grafana employee or OTel community member). I think we can all agree that breaking end users is bad, and that surfacing those issues, regardless of what component they need fixing in, before they reach them is a good idea.

[martincostello]

for example in case of #13727 the source (as mentioned there) does not respect the contact.

If the problem does indeed come from the .NET SDK

Our understanding that this isn't strictly an issue with only the .NET SDK comes from this part of the Protocol Buffers Documentation:

Protocol buffer parsers must be able to parse repeated fields that were compiled as packed as if they were not packed, and vice versa. This permits adding [packed=true] to existing fields in a forward- and backward-compatible way.

This is called out in the description for the PR for the fix made to the OTel Collector for v0.134.0: #13730

If that information is indeed correct (it's implied it is, as the fix was merged and released), then the .NET SDK is implemented in a way that is permissible to the specification, even if that's not the most optimal way to implement the part of the protocol that caused the original incompatibility.

We have recently discussed changing the .NET SDK's OTLP exporter to use the denser format that wasn't affected (open-telemetry/opentelemetry-dotnet#6538 (comment)), but this is a change we'd have to do carefully to ensure we don't create another incompatibility, which leads us back to the issue I opened regarding end-to-end test coverage: open-telemetry/opentelemetry-dotnet#6463

[martincostello]

I haven't heard cases of flakiness within our OATS setup, but @zeitlinger or @martincostello perhaps you could speak to that concern with your experience

I haven't experienced any flakiness inherent to OATs in my usage of it in a personal project (which is what detected the original issue that sparked all this conversation).

---

Flakiness can be a problem for any type of test, though granted they're more likely the higher up the test pyramid you go as the number of moving parts increases - it's something that can be worked to avoid when we get to the point of implementing whatever the agreed solution is.

[zeitlinger]

We can't have integration tests within the collector for every language that has an SDK.

I think that's reasonable - it should not be many tests for each SDK.

In docker-lgtm, we do this for some SDKs already, e.g. https://github.com/grafana/docker-otel-lgtm/blob/main/examples/python/oats.yaml

[bogdandrutu]

If that information is indeed correct (it's implied it is, as the fix was merged and released), then the .NET SDK is implemented in a way that is permissible to the specification, even if that's not the most optimal way to implement the part of the protocol that caused the original incompatibility.

In my humble opinion this is wrong.

I will repeat (again) .net does not respect the protocol, but they have an "escape" patch, because also the protocol says that for future backwards/forward compatibility we should support that. .net MUST send packed repeated scalar fields per the proto3 contract see repeated fields of a primitive type (any [scalar type](https://protobuf.dev/programming-guides/proto2#scalar) that is not string or bytes) are [“packed”](https://protobuf.dev/editions/features#repeated_field_encoding) by default. and in the proto definition there is no "packed=false", see https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/metrics/v1/metrics.proto

See also:

However, if the repeated field is set to expanded (overriding the default packed state) or is not packable (strings and messages) then an entry for each individual value is encoded. Also, records for e do not need to appear consecutively, and can be interleaved with other fields; only the order of records for the same field with respect to each other is preserved.

But we don't have any overwrite of the packed state.