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

telemetry: support lightstep removal and otel addition for envoy tracing #2421

Merged
merged 2 commits into from
Aug 11, 2022

Conversation

douglas-reid
Copy link
Contributor

Envoy has dropped support for the custom Lightstep tracer extension. In the associated PRs, the Lightstep team stated that the path forward is the newly-added OpenTelemetry tracer extension. This PR attempts to appropriately deprecate Lightstep configuration and add a provider for OpenTelemetry (so that it can be exploited by the Telemetry API).

Full issue (for discussion): istio/istio#40027

@douglas-reid douglas-reid requested a review from zirain July 20, 2022 20:44
@istio-testing
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 20, 2022
@istio-policy-bot
Copy link

😊 Welcome @douglas-reid! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 20, 2022
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me

@@ -702,7 +709,7 @@ message MeshConfig {
uint32 port = 2;

// The Lightstep access token.
string access_token = 3;
string access_token = 3 [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.

any special reason for this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can "safely" transition to OTel provider for service + port, but there is no similar concept of access token. That would mean ignoring access_token in this configuration, however.. I was thinking it might be useful to have some way of indicating that it would be ignored in generated Envoy configuration moving forward. But I don't feel too strongly here.

Copy link
Member

Choose a reason for hiding this comment

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

does that means in the next release of istio, Lightstep provider is mostly same as OTel provider?

Choose a reason for hiding this comment

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

Hi @douglas-reid, working on documenting the Lightstep tracer -> OTel migration path. The envoy config setting that replaces access_token is a special header key/value pair, here's a sample envoy config:

            provider:
              name: envoy.tracers.opentelemetry
              typed_config:
                "@type": type.googleapis.com/envoy.config.trace.v3.OpenTelemetryConfig
                grpc_service:
                  envoy_grpc:
                    cluster_name: localcollector
                  initial_metadata:
                    - key: lightstep-access-token
                      value: "your-lightstep-access-token"
                service_name: envoy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smithclay thanks! that's very helpful. whenever you complete the migration guide, we'd really appreciate your sharing it here.

@douglas-reid douglas-reid marked this pull request as ready for review August 10, 2022 00:21
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 10, 2022
@douglas-reid
Copy link
Contributor Author

@howardjohn @zirain updated this API PR. removed the deprecation bits, but added comments about the expected behavioral changes in 1.15+. I think this gives us room to generate proxy-version specific configuration through the transition. Thoughts?

@istio-testing
Copy link
Collaborator

@douglas-reid: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
release-notes_api 63c2aaa link false /test release-notes_api

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@zirain
Copy link
Member

zirain commented Aug 10, 2022

@howardjohn @zirain updated this API PR. removed the deprecation bits, but added comments about the expected behavioral changes in 1.15+. I think this gives us room to generate proxy-version specific configuration through the transition. Thoughts?

we'd better add a warning message in pilot?

@ericvn
Copy link
Contributor

ericvn commented Aug 10, 2022

So this is something we need to cherry-pick to release-1.15 as well?

@istio-testing istio-testing merged commit d4db1bf into istio:master Aug 11, 2022
@ericvn
Copy link
Contributor

ericvn commented Aug 11, 2022

@douglas-reid I'll leave the prior question about if this needs to be cherry-picked up to you.

@douglas-reid
Copy link
Contributor Author

@ericvn i think envoy 1.23 maintains lightstep support in its current form, so I think this is OK waiting for 1.16 (and a full release cycle of burn-in). @smithclay is that a correct interpretation?

@smithclay
Copy link

Correct, breaking envoy change is in envoy 1.24.0.

Any idea what the propagation mode default is for the Lightstep tracer in the current version of Istio? If it's TRACE_CONTEXT, should be mostly good.

ericvn added a commit to ericvn/istio that referenced this pull request Aug 15, 2022
istio-testing pushed a commit to istio/istio that referenced this pull request Aug 15, 2022
* Update latest istio/api and fix lint issues

* Move back to commit before istio/api#2421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants