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

Add back support for Go 1.19 when using Jaeger as a library #4328

Closed
wants to merge 5 commits into from

Conversation

mx-psi
Copy link
Contributor

@mx-psi mx-psi commented Mar 22, 2023

Which problem is this PR solving?

Short description of the changes

This partially reverts #4206 and fully reverts #4291 and #4293.

On the OpenTelemetry Collector project we aim to provide support for all officially supported Go versions. Per Go's support policy, Go 1.19 will remain a supported Go version until Go 1.21 is released.

Since we take a dependency on Jaeger as a library, we would prefer Jaeger to remain compatible with Go 1.19 as a library to be able to keep it updated.

This patch does not revert some changes on #4206; Jaeger binaries would continue to be built with Go 1.20 and benefit from all performance and security improvements that the 1.20 release cycle offers. It does impose the requirement that new code is compatible with Go 1.19 (which sounds okay to me since there was no specific need for Go 1.20 mentioned on the related issues, the only obvious downside is delaying the removal of some helper code).

Signed-off-by: Pablo Baeyens pablo.baeyens@datadoghq.com

This partially reverts jaegertracing#4206.

On the OpenTelemetry Collector project we aim to provide support
for all officially supported Go versions. Per Go's support policy [1],
Go 1.19 will remain a supported Go version until Go 1.21 is released.
Since we take a dependency on Jaeger as a library, we need Jaeger to
remain compatible with Go 1.19 as a library to be able to keep it updated.

This patch does not revert other changes on jaegertracing#4206; Jaeger binaries would
continue to be built with Go 1.20 and benefit from all performance and security
improvements that the 1.20 release cycle offers.

Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
@mx-psi mx-psi marked this pull request as ready for review March 22, 2023 10:57
@mx-psi mx-psi requested a review from a team as a code owner March 22, 2023 10:57
@mx-psi mx-psi requested a review from vprithvi March 22, 2023 10:57
@mx-psi mx-psi marked this pull request as draft March 22, 2023 11:03
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.03 🎉

Comparison is base (6ab3f01) 97.05% compared to head (8de19fa) 97.08%.

❗ Current head 8de19fa differs from pull request most recent head db35d53. Consider uploading reports for the commit db35d53 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4328      +/-   ##
==========================================
+ Coverage   97.05%   97.08%   +0.03%     
==========================================
  Files         300      300              
  Lines       17697    17697              
==========================================
+ Hits        17176    17182       +6     
+ Misses        419      414       -5     
+ Partials      102      101       -1     
Flag Coverage Δ
unittests 97.08% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mx-psi mx-psi changed the title Change go.mod's go directive to indicate support for Go 1.19 Add back support for Go 1.19 when using Jaeger as a library Mar 22, 2023
This reverts commit a172bcc.

Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
…egertracing#4293)"

This reverts commit 18843de.

Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
I added these on a separate file to follow the `ci-unit-tests-go-tip` convention
and to avoid changing the name of the required `unit-tests` check.

Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
@mx-psi
Copy link
Contributor Author

mx-psi commented Mar 22, 2023

@yurishkuro I would love to know if this is something you (as the author/approver of some of the reverted PRs) and the Jaeger project would be willing to accept before I continue working on it. I would need to fix some unit tests, but otherwise this is the extent of the changes needed to add back support for Go 1.19. Would you mind chiming in? Also pinging @jpkrohling whose opinion can be valuable here.

@yurishkuro
Copy link
Member

I'd be fine if this was only the go.mod change, but rolling back so many changes doesn't make sense to me. And frankly neither does the OTEL collector policy - it's not a library where last two versions policy makes sense, collector is a binary, you should be able to upgrade the runtime as you see fit.

@mx-psi
Copy link
Contributor Author

mx-psi commented Mar 22, 2023

@yurishkuro

I'd be fine if this was only the go.mod change, but rolling back so many changes doesn't make sense to me

To be clear, the rest of the changes are just "don't use errors.Join and use the previous thing", but I understand that this is bigger than expected, I will close the PR.

neither does the OTEL collector policy - it's not a library where last two versions policy makes sense, collector is a binary, you should be able to upgrade the runtime as you see fit.

The Collector is consumed as a library for a wide variety of use cases. This includes downstream distros, OpenTelemetry Collector builder users, vendor-specific observability agents like the Datadog Agent or the Grafana Agent or tooling. It's even used as a library by Jaeger itself :)

@mx-psi mx-psi closed this Mar 22, 2023
@yurishkuro
Copy link
Member

I opened #4329 to decide if we want to institute similar policy going forward.

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.

2 participants