[WIP] deps: Remove opentracing #28851
Conversation
repokitteh-read-only
bot
added
api
deps
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
cc @envoyproxy/dependency-shepherds im in favour of landing this immediately we have given ample notice of intention to remove and the upstream project is now archived not sure if there was something else we should have done in terms of formal deprecation, but i think the point is now moot cc @Oberon00 |
|
LGTM. Will be good to see this land from a dependency management view. /lgtm deps |
repokitteh-read-only
bot
removed
the
deps
|
@kyessenov @lizan can you confirm that this won't adversely impact Istio? LGTM if there is no impact on any major set of users. |
While I have been given notice as well as others who stumbled upon this issue, there was never any kind of official announcement or other visibility of this intention outside GitHub issues (please correct me if I'm wrong), nor a deprecation (which I would expect before removing such as feature especially since, as you wrote, this is not an emergency but something where ample time was (is) available to follow the full deprecation path). So even for myself who knew that it was sooner or later going to be removed, I am surprised that this happens suddenly without following the usual way for such configuration items. IMHO you should at least keep the configuration file from breaking, i.e. those who have OpenTracing configured in their bootstrap config should not get an aborted startup but rather a deprecation warning. |
im not sure what you are expecting discussing this offline - i dont think there ever has been a more formal process
it can hardly be called suddenly at this point
im not clear the implications of that exactly, but given that there is no upstream support, i think we are passed deprecation its time for "those who have OpenTracing configured" to update |
I was referring to the normal "Breaking change policy": https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#breaking-change-policy, so with "announcement" I just meant this: "All deprecations/breaking changes will be clearly listed in the version history." Maybe this is a bit unclear in the PR since it just mentions "deps:", but IIUC, this is naturally not only removing the dependency, but introducing a breaking change that removes the feature and will cause configuration errors with configurations that enable this feature. |
sure - we probably should have followed that - even if that is not how dep deprecation have been managed previously to be fair tho - that is something you could have done when we discussed this previously and gave this dep a reprieve for another release cycle as said - the point really is now moot - these deps are archived and need to be removed asap - so this is a lesson for the future |
Probably, but it would have required a lot of effort to find out how to do it. I'm sorry for not being able to put the cycles in.
I don't really understand. The deps have been deprecated/archived already for quite a while (see opentracing/specification#163). I don't understand why this is so urgent now. Also note that the opentracing-cpp repository is not actually archived yet (but that might be just an oversight) |
|
as explained previously these deps have been scheduled for removal for some time - there is now no functioning upstream project, and they are a maintenance headache we agreed to a reprieve in the last release cycle (despite the ongoing headache) - til now not sure there is much else to understand |
|
@htuch about:
I wanted to express that this change will have a large impact on Dynatrace customers using our integration with Envoy today. We are working on an alternative solution, and will open a PR into Envoy once we are ready, that will leverage the existing OpenTelemetry components within Envoy + the OTLP/HTTP exporter being worked on in #28454. So, until that is not ready, customers will be impacted by this. |
There was a problem hiding this comment.
The PR description says that this is a deps change, but it seem like it's much more than than given that it remove code from source/extensions. Please update the PR description to include the full scope of the change.
In addition, from the conversation on this PR it seems like there was some previous communication about this issue. Can you please link to whatever communication/announcements previously took place?
| @@ -19,6 +19,8 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |||
|
|
|||
| // [#protodoc-title: Dynamically loadable OpenTracing tracer] | |||
|
|
|||
| // [#not-implemented-hide:] | |||
| // | |||
There was a problem hiding this comment.
minor point: If I'm understanding correctly, we're keeping around this proto message for configuring OT even though we no longer have code to support it. Should we remove this proto message?
There was a problem hiding this comment.
probably yes - altho i wasnt 100% if we can just to that
There was a problem hiding this comment.
It would be great if the configuration file could keep working (with a deprecation warning maybe) even if it becomes no-op. Otherwise this will break the startup of envoys that configured the now dropped feature.
firstly im wondering why you have waited till now to start resolving given the extensive conversations that have been had previously iiuc this is important to you in terms of released binaries - you want your customers to be able to use the latest envoy with your documented configs so the proposal in this PR is to remove support from 1.28 onwards - that gives you (again - weve been here before) 2+ months to publish and publicise the change for anyone wanting to update to 1.28 when it comes out if thats not the case and you are building a binary yourselves against whatever HEAD then i think you can just revert the pr in your build branch if i do understand correctly then my question is how long do you expect us to maintain an upstream that:
as a maintainer its a headache, we worked around the various issues in the last release cycle, i have failing tests for this right now that i would rather not have to try and debug surely there comes a point where you need to resolve the issue for your customers |
|
Hi @phlax My comment wasn't geared towards blocking this deprecation/removal. I understand the reason behind this, so all the things you mentioned makes sense. Additionally, it is not news to us and we have been working on a solution for our customers for a while now :). The point of my comment was to simply answer @htuch question if this would affect users. Since we do have this data, I thought of sharing it here. |
|
I think we should follow the standard config deprecation process for this one due to acknowledged impact, but immediately transition the filter to unsupported from a security perspective, since there is no sensible way to support something with state as described in opentracing/specification#163. |
|
wfm as long as we start the ball rolling now as discussed offline its not entirely clear what the process should be, but im happy to pr or review |
|
As someone from outside the project, I would expect mostly the process from https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#breaking-change-policy and also user-facing at https://www.envoyproxy.io/docs/envoy/latest/faq/configuration/deprecation, with the only addition that the implementation will immediately switch to be just no-op. I.e. you start with a release that just logs a deprecation warning when the option is used and increases that stat counter, then in the following release it will be a hard error to use the option, unless an override is also used (questionable if this phase makes sense if the feature is no-op, but in case the opentracing config is auto-generated somehow in a way that can't easily turned off by the user, it may be sensible as an escape hatch and a "unmissable" notice) |
|
So it sounds like we're not going to proceed with this PR in it's current form. Is that correct? |
we need to do something, and preferably as early before the next release as possible im not 100% clear what the "formal" process should be, as i think its a path never trodden before for this kind of case. @htuch if you can give me some guidance as to exactly what we can/not disable and how to add deprecations warnings im happy to update and/or create a new PR when we started to look at that seemed non-trivial at least as suggested above - so i think if we are not going to land this now someone needs to propose an interim alternative |
|
Per https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#breaking-change-policy, I think we need to:
We could rework this PR to do this, or open a new PR. |
ping |
|
I think given this discussion the deprecation policy needs to be updated to include Envoy extensions. I will take a stab at it and will post PR shortly. We can then follow it for the a bit more "official" deprecation of opentracing. |
It currently says, "The breaking change policy also applies to source level extensions (e.g., filters)". @yanavlasov, is that insufficient. |
|
Istio does not have OpenTracing API in beta+ stage, so I think we have no concerns there. We'll announce the removal when we release Envoy since someone might be using break-through APIs to access it. |
|
WIPping in favour of #28987 |
phlax
added
no stalebot
|
Final deprecation for |
|
@moderation: Nothing changed in the support/maintenance status of the library, this notice was already in the GH repository description since a long time (see also opentracing/opentracing-cpp#144 (comment)) |
|
@Oberon00 my understanding is we should be gtg with this now - ill rebase this PR |
|
@phlax: Following the envoy project's deprecation policy at https://www.envoyproxy.io/docs/envoy/latest/faq/configuration/deprecation, in 1.30 the config would be annotated in the proto as (NB: From Dynatrace perspective, skipping this hard deprecation aka disallowed_by_default phase would not be a problem but in fact preferable, as you cannot check if a feature is hard-deprecated, but you can check if the binary is compiled with vs without opentracing) |
i think that would be now in terms of dev HEAD
probably we should stick to the deprecation cycle i think - not sure how we can improve that - ive added bubbling for warnings in ci partly with intention of catching issues like this |
Signed-off-by: Ryan Northey <[email protected]>
phlax
force-pushed
the
remove-opentracing
branch
from
eb91321
to
0076362
Compare
repokitteh-read-only
bot
added
the
deps
|
ive added a PR with the hard deprecation here #32421 |
|
closing as completed |
Fix #27401
Related to #28852
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]