-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Conversation
c473dd1
to
b41f56f
Compare
There was a problem hiding this 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
Co-authored-by: Joshua Powers <powersj@fastmail.com>
There was a problem hiding this 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!
There was a problem hiding this 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
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) |
There was a problem hiding this comment.
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):
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!
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
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.
Line 57 in 64c85eb
// 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks!
There was a problem hiding this 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.
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com> Co-authored-by: reimda <reimda@users.noreply.github.com>
I think all the issues are resolved / updated, would be grateful for another look 🙇 |
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 🥳 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 artifactsArtifact URLs |
There was a problem hiding this 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!
Required for all PRs
resolves #11655
Additionally tested locally:
Here is how the userAgent looks:
Message printed during startup: