-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
opentracing: remove legacy open tracing extension #35418
Conversation
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
Signed-off-by: Alyssa Wilk <[email protected]>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
/docs |
Docs for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-pr/35418/docs/index.html The docs are (re-)rendered each time the CI |
@@ -20,10 +20,9 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |||
|
|||
// [#protodoc-title: Dynamically loadable OpenTracing tracer] | |||
|
|||
// DynamicOtConfig is used to dynamically load a tracer from a shared library | |||
// DynamicOtConfig was used to dynamically load a tracer from a shared library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we probably want to make this not-implemented-hide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have provided file level hiding feature and we can hide this file completely. But by the way, why we don't remove this directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it cant be removed as its part of the api, which is essentially immutable (or at least unremovable)
i dont think its hidden currently (or in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now any one use the API will only get an error? (Although I have no strong point to that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think its hidden currently (or in this PR)
Yeah. I mean we can hide this file completely. This is a optional suggestion to @alyssawilk
Updated that comment to make it more clear
Signed-off-by: Alyssa Wilk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can also remove the opentracing dep setup from bazel/repos - but lets do that in a follow up
lgtm, thanks @alyssawilk
Risk Level: medium Testing: n/a Docs Changes: Release Notes: inline Fixes envoyproxy#27401 Fixes envoyproxy#34321 --------- Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Martin Duke <[email protected]>
Risk Level: medium Testing: n/a Docs Changes: Release Notes: inline Fixes envoyproxy#27401 Fixes envoyproxy#34321 --------- Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: asingh-g <[email protected]>
Risk Level: medium
Testing: n/a
Docs Changes:
Release Notes: inline
Fixes #27401
Fixes #34321