Skip to content

tracing: Deprecate OpenTracing#28987

Merged
yanavlasov merged 6 commits intoenvoyproxy:mainfrom
RyanTheOptimist:DeprecateOT
Aug 16, 2023
Merged

tracing: Deprecate OpenTracing#28987
yanavlasov merged 6 commits intoenvoyproxy:mainfrom
RyanTheOptimist:DeprecateOT

Conversation

@RyanTheOptimist
Copy link
Contributor

The OpenTracing upstream project has been abandoned, as noted in #27401.
Consequently this library is not receiving security updates and is not compatible
with Envoy.

Risk Level: N/A
Testing: N/A
Docs Changes: Changelog updated
Release Notes: Changelog updated

Signed-off-by: Ryan Hamilton <rch@google.com>
Signed-off-by: Ryan Hamilton <rch@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #28987 was synchronize by RyanTheOptimist.

see: more, trace.

@RyanTheOptimist
Copy link
Contributor Author

/assign @yanavlasov

Signed-off-by: Ryan Hamilton <rch@google.com>
@yanavlasov
Copy link
Contributor

I think some tests needs to use DEPRECATED_FEATURE_TEST to pass in the compile time options build.

@yanavlasov
Copy link
Contributor

/wait

Signed-off-by: Ryan Hamilton <rch@google.com>
@RyanTheOptimist
Copy link
Contributor Author

I think some tests needs to use DEPRECATED_FEATURE_TEST to pass in the compile time options build.

Oh thanks! I didn't know that was a thing! Done.

@RyanTheOptimist
Copy link
Contributor Author

/retest

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basic approach LGTM.

// <https://github.com/opentracing/opentracing-cpp>`_.
string library = 1 [(validate.rules).string = {min_len: 1}];
string library = 1 [
deprecated = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An observation is that we could set a message-level deprecated annotation, but I'm guessing we would have to update some of the code for warning on this use of deprecated. Not strictly needed here, as to use this filter you need to set some stuff. But, for an empty filter config it would be needed in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had exactly the same thought. I'm inclined to leave that out of this PR and address it if/when we have an empty filter config to deprecate.

Tracing::DriverSharedPtr DynamicOpenTracingTracerFactory::createTracerDriverTyped(
const envoy::config::trace::v3::DynamicOtConfig& proto_config,
Server::Configuration::TracerFactoryContext& context) {
ENVOY_LOG_MISC(warn, "OpenTracing is deprecated and will be removed soon.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be generated anyway via the use of the deprecated proto fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, fair point. I think I misread the suggestion on slack as saying we needed an explicit log in the factory. Reverted.

Signed-off-by: Ryan Hamilton <rch@google.com>
mattklein123
mattklein123 previously approved these changes Aug 15, 2023
Signed-off-by: Ryan Hamilton <rch@google.com>
@yanavlasov yanavlasov merged commit 494c716 into envoyproxy:main Aug 16, 2023
phlax added a commit to phlax/envoy that referenced this pull request Aug 17, 2023
This reverts commit 494c716.

Signed-off-by: Ryan Northey <ryan@synca.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants