go-cloud icon indicating copy to clipboard operation
go-cloud copied to clipboard

server/*: migrate server tracing to OpenTelemetry

Open saeidakbari opened this issue 3 years ago • 14 comments

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 avatar Dec 19 '21 19:12 saeidakbari

Please rescan for CLA to take effect

saeidakbari avatar Dec 19 '21 19:12 saeidakbari

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

Is this a backwards-compatible change?

vangent avatar Dec 22 '21 23:12 vangent

@saeidakbari can you rebase this and sign the CLA?

tmc avatar Sep 12 '22 02:09 tmc

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

saeidakbari avatar Sep 12 '22 15:09 saeidakbari

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 avatar Sep 12 '22 16:09 vangent

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

vangent avatar Sep 12 '22 16:09 vangent

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

saeidakbari avatar Sep 12 '22 17:09 saeidakbari

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.

vangent avatar Sep 12 '22 17:09 vangent

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

saeidakbari avatar Sep 12 '22 18:09 saeidakbari

@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.

saeidakbari avatar Sep 12 '22 20:09 saeidakbari

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

vangent avatar Sep 20 '22 16:09 vangent

@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.

saeidakbari avatar Sep 20 '22 16:09 saeidakbari

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.

vangent avatar Sep 20 '22 16:09 vangent

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.

saeidakbari avatar Sep 20 '22 16:09 saeidakbari

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 https://github.com/ossf/scorecard-webapp/issues/220

naveensrinivasan avatar Oct 01 '22 00:10 naveensrinivasan

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.

vangent avatar Oct 01 '22 00:10 vangent

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

A friendly ping.

naveensrinivasan avatar Oct 26 '22 19:10 naveensrinivasan

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

vangent avatar Dec 28 '22 17:12 vangent