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

[Go] Bump to newer semconv #3838

Merged
merged 2 commits into from
Jan 22, 2024
Merged

[Go] Bump to newer semconv #3838

merged 2 commits into from
Jan 22, 2024

Conversation

pellared
Copy link
Member

@pellared pellared requested review from a team January 22, 2024 08:47
@pellared
Copy link
Member Author

pellared commented Jan 22, 2024

@open-telemetry/go-approvers

Do you think that here

func newResource(serviceName, serviceVersion string) (*resource.Resource, error) {
return resource.Merge(resource.Default(),
resource.NewWithAttributes(semconv.SchemaURL,
semconv.ServiceName(serviceName),
semconv.ServiceVersion(serviceVersion),
))
}

it would be better to use resource.NewSchemaless instead of resource.NewWithAttributes to mitigate problem that the users would have when bumping OTel Go?

Related to open-telemetry/opentelemetry-go#2341

@@ -104,7 +104,7 @@ go get "go.opentelemetry.io/otel" \
"go.opentelemetry.io/otel/sdk/metric" \
"go.opentelemetry.io/otel/sdk/resource" \
"go.opentelemetry.io/otel/sdk/trace" \
"go.opentelemetry.io/otel/semconv/v1.21.0" \
"go.opentelemetry.io/otel/semconv/v1.24.0" \
Copy link
Member

Choose a reason for hiding this comment

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

non blocking comment, @open-telemetry/go-approvers: we can implement an automatic version updating similar to what java, collector, etc have:

Copy link
Member Author

Choose a reason for hiding this comment

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

This could be nice. However, take notice that the version is not equal version of OTel Go. It may not be easy to create such script.

Copy link
Member

Choose a reason for hiding this comment

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

Also for java we have different versions for SDK, semconv, etc. So we might be able to replicate that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also created open-telemetry/opentelemetry-go#4846 so that at least the example would not break in runtime if the code uses an older version of semconv.

@svrnm svrnm added the sig:go label Jan 22, 2024
@cartermp
Copy link
Contributor

@pellared feel free to merge when you feel you've gotten enough feedback from other folks on the Go SIG for your question. Otherwise, I'll plan to merge by end of week.

@pellared
Copy link
Member Author

You’re not authorized to merge this pull request.

@cartermp Can you merge it? This one is a trivial fix. It just propagates the changes: https://github.com/open-telemetry/opentelemetry-go/blob/main/example/dice/otel.go#L29

I will propose a solution that does not require bumping semconv package.. This one would require more approvals from the Go SIG.

@cartermp
Copy link
Contributor

Sounds good.

@cartermp cartermp merged commit f460fb3 into main Jan 22, 2024
16 checks passed
@cartermp cartermp deleted the pellared-patch-1 branch January 22, 2024 20:47
jaydeluca pushed a commit to jaydeluca/opentelemetry.io that referenced this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants