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

server/*: migrate server tracing to OpenTelemetry #3085

Closed
wants to merge 1 commit into from

Conversation

saeidakbari
Copy link

This PR is a step toward migrating Tracing and Metrics from OpenCensus to OpenTelemetry. discussed at #2877

Packages changed are:
server
server/xrayserver
server/sdserver
internal/trace

@saeidakbari
Copy link
Author

Please rescan for CLA to take effect

@vangent
Copy link
Contributor

vangent commented Dec 22, 2021

Sorry, I'm not super familiar with OpenCensus or OpenTelemetry.

Is this a backwards-compatible change?

@tmc
Copy link

tmc commented Sep 12, 2022

@saeidakbari can you rebase this and sign the CLA?

@saeidakbari
Copy link
Author

Sorry for the very late check on this PR.
These changes are not backward compatiable. Is it OK to proceed?

@vangent
Copy link
Contributor

vangent commented Sep 12, 2022

I don't see how it's possible to make this change if it's not backwards compatible. Wouldn't it potentially break all existing users?

@vangent
Copy link
Contributor

vangent commented Sep 12, 2022

(Doesn't OpenTelemetry have a compatibility story with OpenCensus?)

@saeidakbari
Copy link
Author

Practically yes, OpenTelemetry is a successor of OpenCensus but the minimum Go version required for it is 1.16.

@vangent
Copy link
Contributor

vangent commented Sep 12, 2022

The minimum Go version is not a problem.

I think https://opentelemetry.io/docs/migration/opentracing/ describes how to make it backward compatible, by having the OpenTelemetry code also emit OpenCensus events.

@saeidakbari
Copy link
Author

OK, then I think there isn't any issue to continue this PR.
I will make some changes in the next few days.

@saeidakbari
Copy link
Author

@vangent Conflict resolved, but this PR only change the server package to OpenTelemetry.
I did it with the purpose of not making a big PR with lots of code changes.

internal/trace/trace.go Outdated Show resolved Hide resolved
server/requestlog/requestlog.go Show resolved Hide resolved
Signed-off-by: Saeid Akbari <saeidakbari.work@gmail.com>
@vangent
Copy link
Contributor

vangent commented Sep 20, 2022

Related question: why are you motivated to make this change? Can't you use the bridge to get OpenTelemetry events from this library?

@saeidakbari
Copy link
Author

@vangent Accessing OpenCensus data is possible through migrating to OpenTelemetry and implementing the bridge but not vice versa.
Basically, OpenTelemetry is the future of tracing, metrics and monitoring and migrating to it can bring new functionalities in different aspects.

@vangent
Copy link
Contributor

vangent commented Sep 20, 2022

Yes, I think that agrees with what I said. What functionality do you expect to get by doing the migration in this library that you wouldn't get by using the bridge?

My understanding of the compatibility requirements as stated above imply that shared libraries like this one should continue emitting OpenCensus events, supporting both their OpenCensus and OpenTelemetry users, until such a point where there are basically no more OpenCensus users.

I'm not sure how to determine when that point is unfortunately, any ideas? Maybe I can make an announcement that we're planning on making the non-backward-compatible change as part of the next release announcement and see if anyone objects.

@saeidakbari
Copy link
Author

Mostly third-party libraries are more available and maintained for OpenTelemetry rather than deprecated OpenCensus. Also, this migration is inevitable as the process of migration has to happen sometime.

As this project appears to be used in many different places and has a good reputation I think an announcement before the non-backwards change would be good to see if we have any complaints.

@naveensrinivasan
Copy link

Related question: why are you motivated to make this change? Can't you use the bridge to get OpenTelemetry events from this library?

We depend on go-cloud,and this is causing the fuzz testing to fail in oss-fuzz
ossf/scorecard-webapp#220

@vangent
Copy link
Contributor

vangent commented Oct 1, 2022

I did a release today, and included an announcement that we'll be moving to OpenTelemetry in the next release. Unless there are objections over the next couple of weeks, I'll re-review this and plan to get it merged in after that. Thanks for your patience!

Note that I believe you can use the links in comments above to get OpenCensus libraries to emit OpenTelemetry events.

Copy link
Contributor

@vangent vangent left a comment

Choose a reason for hiding this comment

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

Let's go ahead with this. Can you sync to HEAD?

server/server.go Show resolved Hide resolved
@naveensrinivasan
Copy link

Let's go ahead with this. Can you sync to HEAD?

A friendly ping.

@vangent
Copy link
Contributor

vangent commented Dec 28, 2022

Cloned to #3189 , but tests don't pass.

@vangent vangent closed this Dec 28, 2022
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

4 participants