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

Upgrade go.opentelemetry.io/otel from v0.20.0 to v1.11.2 #18589

Merged
merged 2 commits into from
Jan 4, 2023

Conversation

vinay-gopalan
Copy link
Contributor

@vinay-gopalan vinay-gopalan commented Jan 4, 2023

This PR upgrades the following packages:

  • go.opentelemetry.io/otel -> v1.11.2
  • go.opentelemetry.io/otel/sdk -> v1.11.2
  • go.opentelemetry.io/otel/trace -> v1.11.2

This leads to the following changes in indirect dependencies:

  • removed go.opentelemetry.io/otel/metric
  • added github.com/go-logr/stdr v1.2.2

@@ -276,7 +276,7 @@ func (t *TelemetryCollector) getOrBuildResult(id trace.SpanID) *Result {
return r
}

func findAttribute(e trace.Event, attr attribute.Key) string {
func findAttribute(e sdktrace.Event, attr attribute.Key) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Event type previously lived under otel/trace, but has now been moved to otel/sdk/trace instead

@@ -93,7 +93,7 @@ func (s *Session) Finalize(ctx context.Context) *Result {
}

// StartSpan starts a "diagnose" span, which is really just an OpenTelemetry Tracing span.
func StartSpan(ctx context.Context, spanName string, options ...trace.SpanOption) (context.Context, trace.Span) {
func StartSpan(ctx context.Context, spanName string, options ...trace.SpanStartOption) (context.Context, trace.Span) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% sure of the impact of this change. Seems reasonable and looks like it's mainly used for testing, but I could have missed something.

Copy link
Contributor Author

@vinay-gopalan vinay-gopalan Jan 4, 2023

Choose a reason for hiding this comment

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

For further context, this change is required because the function signature for tracer.Start has been updated to accept a SpanStartOption instead of a SpanOption used in (Start is called on L99 and L101).

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks okay to me. The underlying interface for SpanStartOption and SpanEndOption are the same. In v0.20.0, they were just placed into a single SpanOption like:

// SpanOption are options that can be used at both the beginning and end of a span.
type SpanOption interface {
	SpanStartOption
	SpanEndOption
}

Copy link
Contributor

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

LGTM, though we might want to wait until someone who is more familiar with diagnose to approve. Thanks, @vinay-gopalan!

Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

This looks familiar the more I look at it, I'm pretty sure I went through this same upgrade at some point and it just never got merged. LGTM!

@vinay-gopalan vinay-gopalan added this to the 1.13.0-rc1 milestone Jan 4, 2023
@vinay-gopalan vinay-gopalan merged commit 1475ffe into main Jan 4, 2023
@vinay-gopalan vinay-gopalan deleted the upgrade-telemetry-otel branch January 4, 2023 19:31
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

3 participants