envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.93k stars 4.8k forks source link

OpenTracing: common extension code coverage insufficient without client datadog extension #26772

Closed dgoffredo closed 1 year ago

dgoffredo commented 1 year ago

@wbpcode @Shikugawa @basvanbeek from the CODEOWNERS.

I'm working to remove the OpenTracing dependency from Datadog's tracing extension.

The final PR in a series is https://github.com/envoyproxy/envoy/pull/26284. That PR actually removes all references of OpenTracing from the Datadog tracing extension.

As a result, the unit test coverage of extensions/tracers/common/ot has fallen below a threshold enforced by the CI checks. This is probably due to Datadog's unit tests no longer covering code in common/ot.

Here is a coverage report that includes the modified Datadog code: https://storage.googleapis.com/envoy-pr/ec41b8e/coverage/index.html

Here is a coverage report that does not include the modified Datadog code: https://storage.googleapis.com/envoy-pr/45484d5/coverage/index.html

The only difference in coverage of the common/ot source is here: diff

That is, https://github.com/envoyproxy/envoy/blob/4c12d8eda176081a493968709d510065d715347b/source/extensions/tracers/common/ot/opentracing_driver_impl.cc#L68-L75.

I looked into adding a test case to common/ot myself to cover that function, but it's rather involved. It would involve poking opentracing-cpp's MockTracer in just the right way. Maybe you can think of a better approach.

wbpcode commented 1 year ago

I will check it next week. And if it's a reasonable decreasing, you can mark it in the KNOWN_LOW_COVERAGER of per_file_coverage.sh to make the ci pass.

basvanbeek commented 1 year ago

I think it is reasonable to mark as OpenTracing is migrated away from in favor of both OpenTelemetry as well as native instrumentations.

dgoffredo commented 1 year ago

I added another test case to increase the coverage.