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

otlptranslator: fix up import paths #13759

Merged
merged 1 commit into from
Mar 15, 2024
Merged

otlptranslator: fix up import paths #13759

merged 1 commit into from
Mar 15, 2024

Conversation

jcajka
Copy link
Contributor

@jcajka jcajka commented Mar 13, 2024

Fix up inline import paths. These get ignored most of the time by the Go module based tooling, but GOPATH based tooling expects it to match the real import path.

@machine424
Copy link
Collaborator

This is intentional actually, as the files are COPIED AS-IS as mentioned in the header and we want the imports to use that path. See https://go.dev/doc/go1.4#canonicalimports and https://github.com/prometheus/prometheus/tree/main/storage/remote/otlptranslator#copying-from-opentelemetryopentelemetry-collector-contrib
What's exactly does your tooling complain about?

@jcajka
Copy link
Contributor Author

jcajka commented Mar 13, 2024

I don't think that it is intentional, there is shell script that adjusts the import paths for this bit of code https://github.com/prometheus/prometheus/blob/main/storage/remote/otlptranslator/update-copy.sh among other things.

For the record just go test/build running in GOPATH mode not liking it.
otlptranslator/prometheusremotewrite/helper.go:27:2: code in directory /builddir/build/BUILD/prometheus-2.50.1/_build/src/github.com/prometheus/prometheus/storage/remote/otlptranslator/prometheus expects import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheus"
It seems as possible regression since the last bump to this, I haven't bisected it though.

@machine424
Copy link
Collaborator

machine424 commented Mar 13, 2024

Maybe I understood it backwards, but the sed in that file is only for the ./prometheusremotewrite/*.go which doesn't cover the 3 files you changed

"$@" -e 's#github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheus#github.com/prometheus/prometheus/storage/remote/otlptranslator/prometheus#g' ./prometheusremotewrite/*.go
. Plus the README doesn't seem to be up to date anymore (after https://github.com/prometheus/prometheus/tree/main/storage/remote/otlptranslator, we copy /.prometheus as well now)

I'll cc @gouthamve to get a confirmation that the script and the README need to be adapted.
(That would fix the imports)

@aknuds1
Copy link
Contributor

aknuds1 commented Mar 13, 2024

Maybe I understood it backwards, but the sed in that file is only for the ./prometheusremotewrite/*.go which doesn't cover the 3 files you changed

I suspect that update-copy.sh should also fix the import paths in the prometheus/ directory.

@jesusvazquez
Copy link
Member

Yup! I agreee with @machine424 and @aknuds1 , we probably need to update the script to update all the import paths and dont leave any files behind, maybe those files were introduced later and hence the issue.

The reason why we do this with the import paths is that the code from the otel contrib repo still is the source of truth. We cant just import it because there is a cycilical dependency so what we do is try to keep prometheus updated with it. This is not the best scenario but until prometheus becomes the source of truth for that code its what we've got.

@jcajka maybe you'd like to update the script to sed all the imports properly instead?

@jcajka
Copy link
Contributor Author

jcajka commented Mar 13, 2024

I believe that this is the case that the package should be used just under one import path(the canonical one) per the https://go.dev/doc/go1.4#canonicalimports.
Ack, I will fix up the script and I guess the README too, along with the import paths.

Signed-off-by: Jakub Čajka <jcajka@redhat.com>
@jcajka
Copy link
Contributor Author

jcajka commented Mar 13, 2024

Done. Tested by running the script, creates no changes to the tree for me locally.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

LGTM

@gouthamve gouthamve merged commit 1580922 into prometheus:main Mar 15, 2024
24 checks passed
@gouthamve
Copy link
Member

Thanks for your contributions @jcajka!

@jcajka
Copy link
Contributor Author

jcajka commented Mar 18, 2024

Thank you all for quick review and feedback

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

5 participants