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 User-Agent for webhook requests #893

Merged
merged 1 commit into from
Jul 11, 2017

Conversation

kfdm
Copy link
Contributor

@kfdm kfdm commented Jul 4, 2017

Using https://github.com/prometheus/prometheus/pull/2447/files as an example, this adds a user agent for Webhook requests.

This is the first time I've written code in go, so please let me know what I can fix up.
It's likely that the same user agent could be used for the other notifiers but for now I have only added this to the webhook notifier.

@stuartnelson3

Copy link
Contributor

@stuartnelson3 stuartnelson3 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, just one small request!

@@ -135,6 +136,8 @@ func BuildReceiverIntegrations(nc *config.Receiver, tmpl *template.Template) []I

const contentTypeJSON = "application/json"

var userAgentHeader = fmt.Sprintf("Alertmanager/%s", version.Version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put this and the preceeding line into a const declaration block?

const (
        contentTypeJSON = "application/json"
        userAgentHeader = fmt.Sprintf("Alertmanager/%s", version.Version)
)

Copy link
Contributor Author

@kfdm kfdm Jul 10, 2017

Choose a reason for hiding this comment

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

I tried that but I got this error

# github.com/prometheus/alertmanager/notify
notify/impl.go:139: const initializer fmt.Sprintf("Alertmanager/%s", version.Version) is not a constant
!! command failed: build -o ~/.golang/src/github.com/prometheus/alertmanager/alertmanager -ldflags -X github.com/prometheus/alertmanager/vendor/github.com/prometheus/common/version.Version=0.7.1 -X github.com/prometheus/alertmanager/vendor/github.com/prometheus/common/version.Revision=e60ffbe6fb8c58f45864fccd1c741219402c2617 -X github.com/prometheus/alertmanager/vendor/github.com/prometheus/common/version.Branch=user-agent -X github.com/prometheus/alertmanager/vendor/github.com/prometheus/common/version.BuildUser=<removed> -X github.com/prometheus/alertmanager/vendor/github.com/prometheus/common/version.BuildDate=20170710-23:55:18  -s -a -tags netgo github.com/prometheus/alertmanager/cmd/alertmanager: exit status 2
make: *** [build] Error 1

I'm not familiar with Go, so I'm not sure how to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to read up on things, but since const is checked at compile time, but it seems like version.Version is filled in by the linker, it may not be possible to have userAgentHeader as a const (and in the linked PR for Prometheus it's a var there as well)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, consts in Go can't be calculated unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh my mistake, sorry.

@stuartnelson3 stuartnelson3 merged commit dfbac12 into prometheus:master Jul 11, 2017
@kfdm kfdm deleted the user-agent branch July 11, 2017 11:25
@mxinden mxinden mentioned this pull request Jul 15, 2017
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.

3 participants