Skip to content
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

opencensus: Disable by default #33912

Merged
merged 5 commits into from
May 8, 2024
Merged

Conversation

phlax
Copy link
Member

@phlax phlax commented May 1, 2024

Opencensus has been archived for some time upstream, i believe because it was folded in to opentelemetry.

The plan was to remove opencensus and opentracing altogether in the 1.31 release cycle, and both were set to deprecated.

Only opentracing had the disabled_by_default set in time for the release cycle, so this sets it for opencensus.

Original tracking issue is #9958 bu there have been quite a few conversations around this since

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

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 @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #33912 was opened by phlax.

see: more, trace.

@phlax phlax mentioned this pull request May 1, 2024
@phlax
Copy link
Member Author

phlax commented May 1, 2024

cc @kyessenov

kyessenov
kyessenov previously approved these changes May 1, 2024
Signed-off-by: Ryan Northey <[email protected]>
kyessenov
kyessenov previously approved these changes May 1, 2024
@zirain
Copy link
Contributor

zirain commented May 2, 2024

@kyessenov we can/need remove this from istio?

@kyessenov
Copy link
Contributor

@zirain @lei-tang

I think we should remove opencensus-based code from OSS. We are capable of keeping it in Google-vendored Istio builds only, but we probably need to improve our internal CI to handle it.

@kyessenov
Copy link
Contributor

As part of the move from upstream/OSS, we probably want to revert some of these deprecation warnings on the protos as well.

@zirain
Copy link
Contributor

zirain commented May 2, 2024

BTW, the c-ares problem may related any grpc service with dns:xxxxxx(not sure this is the right way to use), we just found it on opencensus.

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Can you please add comments stating the tracking issue/doc that explains why this is no longer supported by default, and how should one migrate?
Please also add a release-note as this is user-facing change.

Signed-off-by: Ryan Northey <[email protected]>
@phlax
Copy link
Member Author

phlax commented May 3, 2024

changelog add and description updated

@adisuissa
Copy link
Contributor

changelog add and description updated

The link to the tracking should be part of the api-doc.
My goal is for some user that detects their old-config is now broken, to look at the field's docs and get a better understanding of what are the steps they should take.

@phlax
Copy link
Member Author

phlax commented May 3, 2024

My goal is for some user that detects their old-config is now broken, to look at the field's docs and get a better understanding of what are the steps they should take.

happy to add that but in 3 months it wont appear at all - wondering if it would be better to just add the link in the changelog

@phlax
Copy link
Member Author

phlax commented May 3, 2024

/docs

Copy link

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/33912/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #33912 (comment) was created by @phlax.

see: more, trace.

@phlax
Copy link
Member Author

phlax commented May 3, 2024

annoyingly deprecation and even default disable are totally ignored https://storage.googleapis.com/envoy-pr/678383c/docs/api-v3/config/trace/v3/opencensus.proto.html

@adisuissa
Copy link
Contributor

happy to add that but in 3 months it wont appear at all - wondering if it would be better to just add the link in the changelog

I need more context. I thought that the api (proto) stays but is no longer implemented (and if that's the case, then the docs will stay, no?)

annoyingly deprecation and even default disable are totally ignored

Aside from the docs there's what happens when running an Envoy and receiving such config (in the bootstrap or dynamically). This will probably break people that use the feature, and I think we need to give them a way to translate their configs. This could probably be in a github issue.

@kyessenov
Copy link
Contributor

kyessenov commented May 3, 2024

@adisuissa This is a case when the library is simply gone and there's no nice way around it. Breakage is imminent, so we're trying to warn developers to do something about it. This should be a lesson not to promote extensions with unstable governance structure - OpenCensus OSS was unilaterally shut down by Google, and all maintenance for legacy use of the library should be deferred back to Google into vendor (closed) code.

@adisuissa
Copy link
Contributor

@adisuissa This is a case when the library is simply gone and there's no nice way around it. Breakage is imminent, so we're trying to warn developers to do something about it. This should be a lesson not to promote extensions with unstable governance structure - OpenCensus OSS was unilaterally shut down by Google, and all maintenance for legacy use of the library should be deferred back to Google into vendor (closed) code.

Sure, and I agree that support will disappear.
What I'm saying is:

  1. give a way for users to migrate to something else (better to write this up in a github issue).
  2. add the link in the fields' comments, so if my config no longer works, I can go to the docs, click the link, and follow the instructions.

There may be some context that I'm missing, and if so feel free to correct me. WDYT?

@kyessenov
Copy link
Contributor

  1. give a way for users to migrate to something else (better to write this up in a github issue).

In general, that something else equivalent doesn't exist. They have to go and buy a solution from a vendor (e.g. Google) to get an extended support for Stackdriver, for example. OpenTelemetry and collector are quite distinct and not easy to migrate to/from.

@adisuissa
Copy link
Contributor

In general, that something else equivalent doesn't exist. They have to go and buy a solution from a vendor (e.g. Google) to get an extended support for Stackdriver, for example. OpenTelemetry and collector are quite distinct and not easy to migrate to/from.

Thanks for clarifying this.
What I'm trying to avoid is unhappy users claiming that Envoy is now broken (bad PR), and us receiving many issues around this (burden on the maintainers).
In other words, put yourself in the user's shoes, and consider if we can make things a bit better (maybe not having a list of steps to solve the issue, but at least have some guidance on what alternatives exist, and where can one find more information about them).

@kyessenov
Copy link
Contributor

@phlax Let's add a release note saying that:

  1. OpenCensus has been sunset, link to https://opentelemetry.io/blog/2023/sunsetting-opencensus/.
  2. Prescribe users to migrate to OpenTelemetry tracer instead, and use an OpenTelemetry collector to convert traces.

@adisuissa
Copy link
Contributor

@phlax Let's add a release note saying that:

  1. OpenCensus has been sunset, link to https://opentelemetry.io/blog/2023/sunsetting-opencensus/.
  2. Prescribe users to migrate to OpenTelemetry tracer instead, and use an OpenTelemetry collector to convert traces.

That would be great. Thanks for finding this Kuat!

phlax added 2 commits May 8, 2024 13:57
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
@repokitteh-read-only repokitteh-read-only bot removed the api label May 8, 2024
@phlax phlax enabled auto-merge (squash) May 8, 2024 18:24
@phlax phlax merged commit d7fbda8 into envoyproxy:main May 8, 2024
52 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants