Skip to content

[WIP] deps: Remove opencensus#28852

Closed
phlax wants to merge 1 commit intoenvoyproxy:mainfrom
phlax:remove-opencensus
Closed

[WIP] deps: Remove opencensus#28852
phlax wants to merge 1 commit intoenvoyproxy:mainfrom
phlax:remove-opencensus

Conversation

@phlax
Copy link
Member

@phlax phlax commented Aug 5, 2023

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:]

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax requested a review from wbpcode as a code owner August 5, 2023 12:43
@phlax phlax marked this pull request as draft August 5, 2023 12:43
@repokitteh-read-only repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Aug 5, 2023
@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 @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @htuch

🐱

Caused by: #28852 was opened by phlax.

see: more, trace.

@phlax phlax changed the title [WIP] deps: Remove opencensus deps: Remove opencensus Aug 5, 2023
@phlax phlax marked this pull request as ready for review August 5, 2023 14:24
@phlax
Copy link
Member Author

phlax commented Aug 5, 2023

cc @envoyproxy/dependency-shepherds

turns out this project is also archived now so i think we should land this immediately

@moderation
Copy link
Contributor

Another nice cleanup

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Aug 6, 2023
@htuch
Copy link
Member

htuch commented Aug 6, 2023

@kyessenov
Copy link
Contributor

This would break Istio unfortunately.

@kyessenov
Copy link
Contributor

CC @lei-tang - we'd need some policy what to do with opencensus in Istio since it's now unmaintained.

@kyessenov
Copy link
Contributor

istio/istio#44743

@lei-tang
Copy link

lei-tang commented Aug 9, 2023

To avoid breaking Istio customers, Istio needs to follow a proper deprecation procedure (e.g., provide an advance notice for the Opencensus deprecation). Before Istio completes its deprecation procedure for Opencensus, it should not be removed.

@moderation
Copy link
Contributor

To avoid breaking Istio customers, Istio needs to follow a proper deprecation procedure (e.g., provide an advance notice for the Opencensus deprecation). Before Istio completes its deprecation procedure for Opencensus, it should not be removed.

Can Istio start that formal process for OpenTracing and OpenCensus now (if it hasn't already).

In parallel, can't Istio continue to ship / use 1.27.0 throughout this deprecation period and no one gets broken?

@kyessenov
Copy link
Contributor

kyessenov commented Aug 10, 2023

Can Istio start that formal process for OpenTracing and OpenCensus now (if it hasn't already).

+1 I think Istio should announce and start emitting warnings.

In parallel, can't Istio continue to ship / use 1.27.0 throughout this deprecation period and no one gets broken?

I don't think that's a good idea as we support Istio versions for several releases, so that would extend 1.27 support window much longer than desirable. Releasing at head of Envoy allows Istio to shorten Envoy support windows.

What could be a temporary workaround is moving the extension to Istio Envoy build if Envoy upstream wants to not be held by Istio.

@nareddyt
Copy link
Contributor

https://github.com/GoogleCloudPlatform/esp-v2 would also be broken by this change. We rely on OpenCensus today.

BTW I see #9958 (Transition to OpenTelemetry from OpenTracing and OpenCensus) is still open. Seems odd to remove support for OpenCensus while transition isn't ready yet.

Also https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/trace/v3/opentelemetry.proto mentions the following:

This extension is work-in-progress. Functionality is incomplete and it is not intended for production use. This extension has an unknown security posture and should only be used in deployments where both the downstream and upstream are trusted.

Are there any tangible steps to get OpenTelemetry tracer extension ready for production use? Is it a matter of adhering to Envoy's extension guidelines, or does it also involve changes to the OpenTelemetry libraries?

https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md#extension-stability-and-security-posture

Ideally this is resolved before deprecating OpenCensus

@lei-tang
Copy link

Advance notice of Istio deprecating Opencensus: istio/istio.io#13715.

@kyessenov
Copy link
Contributor

@nareddyt OpenCensus was in the deprecation announcement mode for years (3?). The immediate issue is that the upstream repo is archived/no longer maintained as of 7/31. Unless someone steps in and forks it, or the downstream projects maintain it, it's not reasonable to expect Envoy to maintain OpenCensus.

Are you using OC for Cloud Trace only? We should probably coordinate the migration path, since it's probably not OpenTelemetry.

@nareddyt
Copy link
Contributor

I see. Correct, we are using it for Cloud Trace. My understanding is that OpenTelemetry is the recommended migration path for tracing, please correct me if I'm wrong.

@kyessenov
Copy link
Contributor

@nareddyt Cloud Trace does not expose an OpenTelemetry endpoint. That means you have to run some collector to proxy telemetry. There's a zipkin collector so you're not limited to Otel for tracing, but it's probably a good idea to start using Otel collector.

@yanavlasov yanavlasov added the no stalebot Disables stalebot from closing an issue label Aug 15, 2023
@phlax
Copy link
Member Author

phlax commented Aug 18, 2023

setting this to WIP in favour of #29063

@phlax phlax changed the title deps: Remove opencensus [WIP] deps: Remove opencensus Aug 18, 2023
@phlax phlax marked this pull request as draft August 18, 2023 14:34
@kyessenov
Copy link
Contributor

@phlax are we removing opencensus this version?

@phlax
Copy link
Member Author

phlax commented Feb 22, 2024

are we removing opencensus this version?

i was going to ping - i think in this version we should do the "hard" deprecation as we have done for opentracing #32421

@kyessenov
Copy link
Contributor

No strong opinion. Istio will have to copy the code anyways when it's gone here since the dependency is used by other filters.

@basvanbeek
Copy link
Member

basvanbeek commented Feb 26, 2024

Has any outreach to customers and major depending projects have happened?

I know we have said it would be removed in 1.30 and that has been messaged in release notes, I do notice that on the actual documentation site we don't even mention this in the API descriptions. It is very easy for people to have this information go unnoticed.

@phlax
Copy link
Member Author

phlax commented Feb 27, 2024

@basvanbeek its due to be actually removed in 1.31 now.

The plan for 1.30 is just hard deprecation - ie disabled by default

I believe some comms were sent out - not 100% - but it was marked deprecated, and the conversation has been pretty active here on github

@phlax
Copy link
Member Author

phlax commented Dec 3, 2024

cc @ggreenway - i can reactivate this now i think

@ggreenway
Copy link
Member

Yep, it's time. Do you want to bring this PR up to date, or should we start fresh?

@ggreenway
Copy link
Member

This is superseded by #37508

@ggreenway ggreenway closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api no stalebot Disables stalebot from closing an issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants