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

Have a ForwardPropagator for istio #877

Closed
romilpunetha opened this issue May 4, 2020 · 12 comments · Fixed by #1901
Closed

Have a ForwardPropagator for istio #877

romilpunetha opened this issue May 4, 2020 · 12 comments · Fixed by #1901
Labels
area:trace Part of OpenTelemetry tracing question Further information is requested wontfix This will not be worked on

Comments

@romilpunetha
Copy link

When using Otel with istio, the following seems to break the traces:

A --> envoyA(sets headers) --> envoyB(checks and forwards headers) --> B -> Otel(updates spanId and parentSpanID) --> envoyB(checks and updates headers) --> envoyC(checks and forwards headers) -->C

When Otel performs substitution in headers, envoy does it too for all outgoing requests, and sets the parentSpanId to the spanId created by Otel, which was never exported. For every subsequent span, theres an error that parentSpanId doesn't exist as shown in the image.

Screenshot 2020-05-05 at 1 06 38 AM

Having a ForwardPropagator that only forwards the headers in outgoing requests solves this problem.

@evantorrie
Copy link
Contributor

@romil-punetha Which language are you using for your Otel implementation?

@bogdandrutu bogdandrutu transferred this issue from open-telemetry/opentelemetry-specification Jun 26, 2020
@bogdandrutu
Copy link
Member

I think Istio is in GO so moved there, if that is not the case please let me know.

@andrewhsu
Copy link
Member

From our triage session today, this issue seems like it would be more suitable for opentelemetry golang sig which is why it was moved.

@MrAlias
Copy link
Contributor

MrAlias commented Jun 29, 2020

The default tracer is a No-Op tracer, it should implement the behavior you are describing.

@romil-punetha are you registering a non-No-Op tracer in your code? Do you have a link to the service code you are displaying above, or maybe an example case that displays the issue?

@Aneurysm9
Copy link
Member

I'm not sure that I understand the request here. Propagators provide a mechanism for extracting trace context data from, and injecting it into, metadata carriers such as HTTP headers. Those need to be used by an integration with a transport mechanism, such as those provided by the othttp package. This instrumentation participates in the trace by creating its own spans to represent the work performed by that application component. If such an instrumentation is used then the SDK in the application component using that instrumentation needs to be configured with an appropriate trace exporter so that the spans created by the instrumentation can be correlated with the other spans in the trace.

If, however, what you're looking for is an instrumentation that does not participate in the trace but merely forwards the trace context information then that is perhaps best achieved at an application level by selecting the trace headers to forward at the time a request is received, storing them in a context, and adding them to outbound requests unmodified. I don't believe the default W3C TraceContext propagator provided by the SDK will be useful for this as the Extract method stores the extracted SpanContext with the remoteContextKey and the Inject method expects an active local SpanContext stored using the currentContextKey.

It seems odd to me that we would want to create and maintain a system that expects users to configure an OTel SDK in a manner that takes equivalent effort to actually instrumenting an application and participating in a trace but does not and merely forwards trace context information. Absent a requirement from the specification, I would be opposed to taking on that maintenance overhead when the better course of action is to ensure developers are able to instrument their applications in a way that makes them full participants in the distributed trace.

@MrAlias
Copy link
Contributor

MrAlias commented Jun 30, 2020

Might be a duplicate of #765

@evantorrie
Copy link
Contributor

The Istio control plane is written in Go, but the data plane including Envoy is in C++.

This is not the first time I've heard an ask like this regarding Istio.

I believe the "distributed tracing via the Service Mesh" is often one of the selling points of Istio, i.e. just auto-inject this Envoy sidecar proxy into every k8s pod, and it will handle distributed tracing for you. What sometimes isn't realized until further investigation is that in order for any of the Envoy-generated spans to participate in the same trace, it is the application's responsibility to at least copy incoming trace headers to the outgoing request.

Even without any internal application span generation, I do think there's some value for Istio-based deployments to benefit from a full "service-to-service" trace managed by the Envoy sidecars, which they would do if there were some sort of "forwarding-only" propagator.

One possible hacky solution to this which I don't think is explicitly outlawed by the spec, but may need some sort of configuration opt-in, is to change propagation.HTTPPropagator's Inject(context.Context, propagation.HTTPSupplier) semantics to fallback to an existing RemoteSpanContext if there is no current Span in the supplied context.

For example, if the existing span context extraction were changed to

	sc := SpanFromContext(ctx).SpanContext()
	if !sc.IsValid() {
		sc = RemoteSpanContextFromContext(ctx)
		if !sc.IsValid() {
			return
		}
	}

I think this would effectively operate as a "passthrough headers" mechanism assuming the propagators were configured correctly. Currently that would likely be the B3 propagator for Istio as there is not an OpenTelemetry tracing addon for Envoy at this point in time.

@Aneurysm9
Copy link
Member

I question whether providing a "passthrough headers" mechanism for Istio is something this project should concern itself with. I believe that the effort required to configure any passthrough mechanism would be equivalent to the minimal effort required to actively participate in the trace. The user would need to configure a propagator to be invoked on every incoming request or use an integration such as othttp, use one of the instrumentations from contrib, or manually extract the relevant context on each request (though the pre-existing instrumentations all create new spans for inbound requests). The user would need to configure a propagator to be invoked in an http.Transport to ensure outgoing headers were set (we provide one in othttp, but it creates new spans for outbound requests), or set them manually on every request. The only step elided is configuring an exporter to ensure the trace graph isn't broken. If the spans generated by Istio are useful then it means a collection backed already exists somewhere.

@evantorrie
Copy link
Contributor

Yes, that's fair enough. It does seem like maintenance effort for something that most devs would end up just switching eventually to the span creation + trace exporter approach. It may seem more daunting due to the still nascent documentation, so perhaps a blog post on OpenTelemetry in Istio would help explain that the increased amount of effort to get full internal span generation is minimal compared to trying to use OpenTelemetry to propagate just the headers.

@romil-punetha Do you have more context on issues that may exist in instantiating a span exporter that sends the internally generated traces to the same collector that the Istio Envoy sidecars do?

@MrAlias MrAlias added enhancement New feature or request question Further information is requested area:trace Part of OpenTelemetry tracing labels Jul 2, 2020
@evantorrie
Copy link
Contributor

With no response from original poster, I suggest we close this issue as Won't Do.

@MrAlias MrAlias closed this as completed Aug 15, 2020
@MrAlias MrAlias added wontfix This will not be worked on and removed enhancement New feature or request labels Aug 15, 2020
@lavalamp
Copy link

A super quick observation:

  • Kubernetes offers a client library
  • The client library is used by e.g. users writing webhooks
  • It'd be valuable if such webhooks at least passed through spans, even if they don't give other info
  • It doesn't seem at all fair to make a decision for users about whether or how they should be configuring span reporting
  • So I think it'd be good if there were an extremely tiny dependency that could be used to propagate spans without doing anything else

@dashpole
Copy link
Contributor

For anyone that was interested in this use-case, it is now possible to pass through context without writing spans. See https://github.com/open-telemetry/opentelemetry-go/tree/main/example/passthrough for an example, and explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants