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

Replace OpenCensus with OpenTelemetry #2136

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

akerouanton
Copy link

@akerouanton akerouanton commented May 6, 2024

This PR is almost ready to review but I'd like to see if/where the CI fails before putting it out of draft mode.


The OpenCensus repo has been archived on July 31, 2023 (see here), and thus will receive no further updates. OTel offers a bridge to help transition however this means every project using hcsshim need to set up that bridge. At this point, it seems better to transition to OTel instead.

This commit fully replaces OpenCensus with OpenTelemetry v1.21.

  • Package internal/oc has been replaced by internal/otelutil.
  • octtrpc has been replaced by otelttrpc.
  • go.opencensus.io/plugin/ocgrpc has been replaced by go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.
  • Whenever possible, span attributes are passed directly when the span is created.

@akerouanton
Copy link
Author

@microsoft-github-policy-service agree

The OpenCensus repo has been archived on July 31, 2023 (see [1]),
and thus will receive no further updates. OTel offers a bridge to
help transition however this means every project using hcsshim
need to set up that bridge. At this point, it seems better to
transition to OTel instead.

This commit fully replaces OpenCensus with OpenTelemetry v1.21.

- Package `internal/oc` has been replaced by `internal/otelutil`.
- `octtrpc` has been replaced by `otelttrpc`.
- `go.opencensus.io/plugin/ocgrpc` has been replaced by `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`.
- Whenever possible, span attributes are passed directly when the
span is created.

[1]: https://opentelemetry.io/blog/2023/sunsetting-opencensus/

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Previous commit made clear that `(*externalProcess).Wait()` was
missing a call to `span.End()`.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@akerouanton akerouanton marked this pull request as ready for review May 21, 2024 13:53
@akerouanton akerouanton requested a review from a team as a code owner May 21, 2024 13:53
@kevpar
Copy link
Member

kevpar commented Jul 5, 2024

Hi @akerouanton, thanks for sending this PR!

Currently we have some internal usage of the OC spans, so I'd like to make sure we have a smooth transition from OC to OTel to ensure nothing breaks. So, my current thinking is we should do something like the following:

  • Abstract all current OC span creation (maybe internal/span) behind an internal package, such that no OC specific packages need to be imported in most of the code.
  • In internal/span, add support to export to OTel. Have a package config setting that controls whether spans are exported to OC, OTel, or (potentially) both.

With this in place, we'd be able to get OTel support merged, but keep internal usage going to OC until we can validate that the OTel work won't break anything.

As a plus, this approach also gives us a way to potentially inject other span-wide instrumentation, such as using pprof.WithLabels.

Let me know if you have thoughts on this approach.

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.

None yet

2 participants