datadog: Config (final) portion of new tracing library

[dgoffredo]

This is the final part of the work proposed in #23958.

This PR additionally contains all of the changes from #26148. This diff will shrink when that PR is merged.

This revision alters the DatadogTracerFactory in source/extensions/tracers/datadog/config.h to produce instances of the new dd-trace-cpp based implementation of the Datadog tracer, instead of the old dd-opentracing-cpp based one.

This revision actually alters the implementation of the Datadog tracer. Previous pull requests have only added code for use here.

This revision also moves some unit tests around:

• Two tests that were part of datadog_tracer_impl_test.cc have been moved into agent_http_client_test.cc, because they're about HTTP requests.
• One test that was part of datadog_tracer_impl_test.cc has been removed because its behavior is already covered by tests added in other PRs in this series.
• The remaining tests in datadog_tracer_impl_test_.cc were moved into config_test.cc, since they all now have to do with the configuration of the tracer and the resulting behavior.

Finally, all references to dd-opentracing-cpp and msgpack have been removed from the repo, and the documentation (READMEs) and demo/ directory from #23958 have been added.

[dgoffredo]

I'm currently working on modifications to the common/ot code that might resolve this. It will require a little circumvention but will probably come out fine.

[...] AFAICT there is no dependence on this other than datadog, and it is not adequately tested on its own.

Another option which also makes sense to me is to, in this PR, delete the ot directories and their files. That would be more extreme, but it would wake up anyone that cares to re-add this (and contribute tests to unblock that PR).

common/ot is depended upon additionally by dynamic_ot:

envoy/source/extensions/tracers/dynamic_ot/BUILD

Line 24 in 6d6661c

"//source/extensions/tracers/common/ot:opentracing_driver_lib",

The relevant feature is documented here: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/trace/v3/dynamic_ot.proto

It allows a user to bring their own build of an OpenTracing-compatible tracer and dlopen it into Envoy on start.

🤷 If you're willing to remove that feature, then the common/ot code can be removed also.

[wbpcode]

As @wbpcode is an owner of this extension WDYT?

In fact, I have no strong point to this. Because I don't know if there are actual users of common/ot or dynamic_ot. I had never got related feedbacks from community, if my memory is correct.

If the community think we should keep these extensions, I can add some tests to improve the coverage in the next week and unblock this PR (I will have some free time at that time.)

[wbpcode]

I'm currently working on modifications to the common/ot code that might resolve this. It will require a little circumvention but will probably come out fine.

If you can resolve it directly it would great.

[dgoffredo]

I pushed a commit that covers the following code:

envoy/source/extensions/tracers/common/ot/opentracing_driver_impl.cc

Lines 68 to 75 in 4c12d8e

	opentracing::expected<void> ForeachKey(OpenTracingCb f) const override {
	trace_context_.forEach([cb = std::move(f)](absl::string_view key, absl::string_view val) {
	opentracing::string_view opentracing_key{key.data(), key.length()};
	opentracing::string_view opentracing_val{val.data(), val.length()};
	return static_cast<bool>(cb(opentracing_key, opentracing_val));
	});
	return {};
	}

by adding some new boilerplate to the common/ot unit test: a7bf4fd

The difficulty is that the existing unit test uses opentracing::mocktracer::MockTracer to interact with the driver under test. opentracing::mocktracer::MockTracer will not call the "for each" flavor of header extraction. It's just not in the opentracing-cpp code. opentracing-cpp is deprecated and hasn't been touched in a while, so I don't think modifying that library and cutting a release is a viable option.

Instead, I reproduce a skeleton of an opentracing::Tracer (together with its opentracing::Span and opentracing::SpanContext) that does nothing except call the "for each" flavor when extraction is requested.

[alyssawilk]

sweet! No changes to core code and no changes to coverage so I'll recuse myself and let josh merge.

[alyssawilk]

looks like clang-tidy failed though so you may want to clean that up so it' good to merge :-)

[dgoffredo]

@alyssawilk we are go for launch, stand by for ignition sequence

[jmarantz]

@alyssawilk has deferred to me. LGTM!

all requests are addressed.

[dgoffredo]

🎉🎉🎉🎉🎉🎉🎉🎉

Once again, thank you @jmarantz, @wbpcode, and everyone who contributed to the review of these 10 pull requests.

I learned a lot and am looking forward to collaborating again.

[dgoffredo]

@moderation

This is a Datadog call, however the existing extension is available in the 1.25.x release cycle. The new extension would be in the 1.26x release chain. Seems OK vs. a flag (and then flag maintenance)

I downloaded the zipped source code from the v1.26.1 release, and it does not include the changes in this PR, thus v1.26.1 contains the old extension, not the new one.

Might a subsequent release in the v1.26.x series contain these changes, or will we have to wait for v1.27.x? I'd prefer to have these changes available as soon as possible. The changes were initially proposed in November of 2022.

edit: It looks like we missed the release cutoff by six hours!

[phlax]

Might a subsequent release in the v1.26.x series contain these changes

its possible we could backport the changes to 1.26.x, but i think that we should not as that is usually reserved for fixes

the 1.27 release is due for 2023/07/14

[dgoffredo]

I agree that a backport would not be appropriate here. Ok, July it is. Thanks @phlax