Skip to content

Conversation

@jsoriano
Copy link
Contributor

@jsoriano jsoriano commented Feb 13, 2025

Description

Migrate from OpenCensus to OpenTelemetry, doing some adjustements where there is no 1:1 migration path:

  • zpages handler explicitly added to http.DefaultServeMux, as zpages.Handle() is not available anymore.
  • Annotations replaced by Attributes.

I am also adjusting some uses of spans in compactions, so their annotations are not overwritten on every call to runCompactDef in cases where it is called multiple times.

Not using latest version of OpenTelemetry because it requires Go 1.22, and badger still supports 1.21.

Fix #2155.

Checklist

  • Code compiles correctly and linting passes locally
  • For all code changes, an entry added to the CHANGELOG.md file describing and linking to
    this PR
  • Tests added for new functionality, or regression tests for bug fixes added as applicable
  • For public APIs, new features, etc., PR on docs repo staged and linked here

@jsoriano jsoriano requested a review from a team February 13, 2025 13:35
@jsoriano
Copy link
Contributor Author

Not sure how to address the issue with protobuf, could it be that protoc has a different version in CI? 🤔

@mauri870
Copy link

mauri870 commented Feb 13, 2025

About the Go version, I see #2125 downgraded it from 1.23 to 1.21. Even if the repo goes with the last N-2 versions as opposed to the Go release policy of N-1 that would still be valid to bump it to 1.22 now. Let's wait for a maintainer's input on this.

@mangalaman93
Copy link
Contributor

We have upgraded the Min Go version to 1.22 here #2171. Please upgrade the opentelemetry to the latest and we would be happy to merge the PR. Thank you for creating the PR.

@jsoriano
Copy link
Contributor Author

OTel dependencies upgraded.

Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Minor spelling mistake! LGTM otherwise. @harshil-goel for final review before merge.

Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

two more comments, we are good to merge otherwise! And resolve the conflicts as well. Thank you.

@mangalaman93 mangalaman93 enabled auto-merge (squash) February 17, 2025 17:35
@ryanfoxtyler ryanfoxtyler merged commit 3568af4 into dgraph-io:main Feb 17, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE]: Change opencensus to opentelemetry

4 participants