-
Notifications
You must be signed in to change notification settings - Fork 597
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
Port opencensus integration to opentelemetry #3126
Comments
Related to knative/pkg#855 |
Oh thanks for spotting it! |
Looks like OpenTelemetry Go SDK is in beta now: https://github.com/open-telemetry/opentelemetry-go. So the next step is making sure that OT meets all our requirements. |
@grantr do we have some document with these requirements defined? How would you proceed? |
@evankanderson @anniefu would you mind sharing what you had in mind? I know you had done a lot of work here. |
The upcoming metrics plan that Evan wrote up https://github.com/knative/pkg/blob/master/metrics/README.md is primarily about moving to a centralized collector service model for metrics. Since, the OpenCensus Collector Service agent and the OpenTelemetry Collector Service agent will both support the OpenCensus client libraries, we can port from opencensus client libraries to the opentelemetry client libraries independently of the new metrics plan. The last time we talked to OpenTelemetry back in february, their metrics models were still in flux and they recommended @evankanderson make changes to opencensus library to support Knative rather than trying to early-migrate to Otel client libraries. Given that, I think we want the metrics plan to land and maybe wait beyond Otel beta before considering porting Knative libraries. |
In terms of knative/eventing-contrib#1155, tracing and metrics in Knative are handled completely separately right now, so it is possible to migrate to OpenTelemetry for tracing, but not metrics. I'm not very familiar with the world of tracing, so I'll leave that judgment call to others. |
Links to cloudevents/sdk-go#554 |
This issue is stale because it has been open for 90 days with no |
/remove-lifecycle stale |
We discussed this a bit yesterday (meeting minutes here), I will take a look by porting the Api Server source to the new model and then we can discuss the next steps. |
@skonto that sounds great, can you please create an issue for that ? |
Sure @devguyio |
@skonto is this issue still useful? Or is there an updated one and we can close this one? |
@slinkydeveloper let's keep this is an umbrella ticket for the Eventing migration work. The other ticket is just a sub-task. |
This issue is stale because it has been open for 90 days with no |
hi all, I wonder when we could expect eventing to support OpenTelemetry. |
@skonto thank you. OpenTelemetry traces for Serving and our gRPC based functions work well. With eventing, we are able to visualize Knative components but we do not know how to add the functions (also deployed with Serving), which generate ClouEvents and get triggered by them, to the same traces. I wonder if there are any examples for that. |
@ustiugov cloudevents client is observability aware so if you try setup tracing as in https://knative.dev/docs/eventing/accessing-traces/ you should see them for existing components. Same applies to user services that utilize the same api and setup tracing accordingly: https://github.com/cloudevents/sdk-go/blob/release-2.1/samples/http/receiver-traced/main.go. For a concrete trace handler using opencensus check: eventing/pkg/adapter/v2/cloudevents.go Line 60 in 2fcbc0a
Not sure if there are more samples maybe @slinkydeveloper has more pointers. |
This issue is stale because it has been open for 90 days with no |
/remove-lifecycle stale |
This issue is stale because it has been open for 90 days with no |
/remove-lifecycle stale |
Problem
Now we use opencensus, which is going to be deprecated, we need to switch to https://github.com/open-telemetry/opentelemetry-go
Persona:
Knative contributor
Exit Criteria
We don't depend anymore on opencensus
Time Estimate (optional):
🤷♂️
Additional context (optional)
We also need to handle this: knative/eventing-contrib#1155 (comment)
The text was updated successfully, but these errors were encountered: