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

fix: refactor telegraf version #11656

Merged
merged 6 commits into from
Aug 17, 2022
Merged

Conversation

povilasv
Copy link
Contributor

@povilasv povilasv commented Aug 10, 2022

Required for all PRs

resolves #11655

Additionally tested locally:

Here is how the userAgent looks:

telegraf 1.24.0-35803efd (linux/amd64)`
./telegraf --version             
Telegraf 1.24.0-35803efd (git: master 35803efd)

Message printed during startup:

/telegraf --config telegraf.conf
2022-08-10T08:32:05Z I! Starting Telegraf 1.24.0-35803efd

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Aug 10, 2022
@povilasv povilasv force-pushed the version branch 2 times, most recently from c473dd1 to b41f56f Compare August 10, 2022 08:35
@povilasv povilasv marked this pull request as ready for review August 10, 2022 09:42
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thanks for this! Two comments

Makefile Show resolved Hide resolved
internal/internal.go Outdated Show resolved Hide resolved
Co-authored-by: Joshua Powers <powersj@fastmail.com>
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @povilasv for the nice rework!

@srebhan srebhan self-assigned this Aug 11, 2022
@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 11, 2022
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Just one small comment...

Makefile Outdated
Comment on lines 48 to 49
BUILD_INFO_IMPORT_PATH=github.com/influxdata/telegraf/internal
LDFLAGS := $(LDFLAGS) -X $(BUILD_INFO_IMPORT_PATH).commit=$(commit) -X $(BUILD_INFO_IMPORT_PATH).branch=$(branch) -X $(BUILD_INFO_IMPORT_PATH).goos=$(GOOS) -X $(BUILD_INFO_IMPORT_PATH).goarch=$(GOARCH)
Copy link
Member

Choose a reason for hiding this comment

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

We no longer have internal.goos and internal.goarch so I think this should be (with shortened variable):

Suggested change
BUILD_INFO_IMPORT_PATH=github.com/influxdata/telegraf/internal
LDFLAGS := $(LDFLAGS) -X $(BUILD_INFO_IMPORT_PATH).commit=$(commit) -X $(BUILD_INFO_IMPORT_PATH).branch=$(branch) -X $(BUILD_INFO_IMPORT_PATH).goos=$(GOOS) -X $(BUILD_INFO_IMPORT_PATH).goarch=$(GOARCH)
INTERNAL_PKG=github.com/influxdata/telegraf/internal
LDFLAGS := $(LDFLAGS) -X $(INTERNAL_PKG).commit=$(commit) -X $(INTERNAL_PKG).branch=$(branch)

If you accept the shortened name, please also change below!

Copy link
Contributor Author

@povilasv povilasv Aug 12, 2022

Choose a reason for hiding this comment

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

One question should I leave the goos and goarch LDFLAGS? As this is suggested by @powersj in
#11656 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@srebhan this is where you come by and say yes :)

Copy link
Member

Choose a reason for hiding this comment

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

Ummm yes? :-) I'm not against leaving it in, but I'm not sure what happens if there is no such variable defined? What is the purpose of this?

Copy link
Contributor

@powersj powersj Aug 12, 2022

Choose a reason for hiding this comment

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

The purpose of leaving it in is to keep the current behavior, but more importantly, to clearly see that the variable is set or when it is set when running build commands.

edit: to add a bit more, I want to see when I or others build telegraf, what those variables, GOOS, GOARCH, are set to. Because we do modify those variables in the makefile and allow users to cross-build, knowing exactly what the make build command receives as these values is incredibly important. So keeping them somewhere that shows up is important.

Copy link
Contributor

Choose a reason for hiding this comment

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

@srebhan and I chatted and I think we now agree on removing GOOS and GOARCH for now.

I think we only need to replace all the BUILD_INFO_IMPORT_PATH with INTERNAL_PKG as @srebhan suggested above.

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
"github.com/influxdata/telegraf/plugins/common/tls"
"github.com/influxdata/telegraf/plugins/outputs"
)

var userAgent = fmt.Sprintf("telegraf (%s/%s)", runtime.GOOS, runtime.GOARCH)
var userAgent = fmt.Sprintf("telegraf %s (%s/%s)", internal.Version(), runtime.GOOS, runtime.GOARCH)
Copy link
Contributor

Choose a reason for hiding this comment

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

All plugins should be using the same user agent. The project has standardized on internal.ProductToken() as the user agent.

// ProductToken returns a tag for Telegraf that can be used in user agents.

I see that the opentelemetry plugin wasn't using it before, but if we're changing the plugin, we should be switching to the standard, not adding a different user agent format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

Makefile Show resolved Hide resolved
internal/internal.go Outdated Show resolved Hide resolved
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Please also replace the two usages with the new variable names.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
povilasv and others added 2 commits August 17, 2022 12:44
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Co-authored-by: reimda <reimda@users.noreply.github.com>
@povilasv
Copy link
Contributor Author

I think all the issues are resolved / updated, would be grateful for another look 🙇

@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

🥳 This pull request decreases the Telegraf binary size by -1.08 % for linux amd64 (new size: 148.6 MB, nightly size 150.2 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_i386.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz
static_linux_amd64.tar.gz

Copy link
Contributor

@MyaLongmire MyaLongmire left a comment

Choose a reason for hiding this comment

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

This is great. Thanks!

@MyaLongmire MyaLongmire merged commit 447e8a3 into influxdata:master Aug 17, 2022
@povilasv povilasv deleted the version branch August 29, 2022 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow adding version to userAgent
5 participants