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

Use the new prometheus http API #71

Merged
merged 3 commits into from
Apr 30, 2017

Conversation

juergenhoetzel
Copy link
Contributor

prometheus/client_golang#214

Thanks for this package! 👍

The new recommended way of using the prometheus http handler is through
the promhttp package:

https://github.com/prometheus/client_golang/releases/tag/v0.8.0 (Separated
HTTP exposition, allowing custom HTTP handlers (package promhttp))
@carlpett
Copy link
Collaborator

Hi @juergenhoetzel, thanks for the PR! The build fails because of missing vendoring, could you add promhttp as a vendored package?

@martinlindhe
Copy link
Collaborator

martinlindhe commented Apr 30, 2017

@juergenhoetzel Good work! For the pr to land, you'll also need to commit the changes in vendor, because it is still failing on missing promhttp. Appveyour output: https://ci.appveyor.com/project/martinlindhe/wmi-exporter/build/258/job/x5ucqsbxku8huu96

govendor test -v +local
exporter.go:18:2: cannot find package "github.com/prometheus/client_golang/prometheus/promhttp" in any of:
	c:\gopath\src\github.com\martinlindhe\wmi_exporter\vendor\github.com\prometheus\client_golang\prometheus\promhttp (vendor tree)
	C:\go\src\github.com\prometheus\client_golang\prometheus\promhttp (from $GOROOT)
	c:\gopath\src\github.com\prometheus\client_golang\prometheus\promhttp (from $GOPATH)
Error: exit status 1

if err := http.ListenAndServe(*listenAddress, nil); err != nil {
log.Fatalf("cannot start WMI exporter: %s", err)
}
log.Fatalf("cannot start WMI exporter: %s", http.ListenAndServe(*listenAddress, nil))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this is correct.
log.Fatal() simply exits the program. You'll still need to do as before, that is checking for error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @martinlindhe
http.ListenAndServer only returns when an error occured. I.e. it never returns nil. This style is also used in the official Go http documentation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that's all good then! I was under the wrong impression it was non-blocking

.gitignore Outdated
VERSION
vendor/*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong. We do version the vendor folder.

appveyor.yml Outdated
@@ -21,6 +21,7 @@ install:

build_script:
- ps: gitversion /output json /showvariable FullSemVer | Set-Content VERSION -PassThru
- govendor sync
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong. We commit the vendor folder for reproducible builds.

@juergenhoetzel
Copy link
Contributor Author

@juergenhoetzel Good work! For the pr to land, you'll also need to commit the changes in vendor, because it is still failing on missing promhttp. Appveyour output: https://ci.appveyor.com/project/martinlindhe/wmi-exporter/build/258/job/x5ucqsbxku8huu96

@martinlindhe Thanks for the hint. I didn't used govendor before. I removed all vendored sources and and used the sync functionality instead: https://github.com/kardianos/govendor .

@martinlindhe
Copy link
Collaborator

martinlindhe commented Apr 30, 2017

@juergenhoetzel For vendoring, all you need to do is, manually

govendor sync
git add vendor
git commit -am "updated vendored dependencies"

This gives us a working snapshot of correct vendored dependencies,
so we have reproducible builds.
We don't want this step to happen in appveyor at a later time, when deps might have changed, and we don't want the vendor folder gitignored.

@juergenhoetzel
Copy link
Contributor Author

This gives us a working snapshot of correct vendored dependencies,
so we have reproducible builds.
We don't want this step to happen in appveyor at a later time, when deps might have changed, and we don't want the vendor folder gitignored.

Make sense. I will revert/amend the change.

@martinlindhe
Copy link
Collaborator

martinlindhe commented Apr 30, 2017

Feel free to just commit additional changes to this branch (if that is less work), we can merge the PR as a single commit when done

@martinlindhe
Copy link
Collaborator

I seem to have forgotten how to use govendor.

I think the proper usage is something like:

govendor fetch github.com/prometheus/client_golang/prometheus/promhttp
git add vendor
git commit -am "updated vendored dependencies"

@juergenhoetzel
Copy link
Contributor Author

I seem to have forgotten how to use govendor.

I think the proper usage is something like:

govendor fetch github.com/prometheus/client_golang/prometheus/promhttp
git add vendor
git commit -am "updated vendored dependencies"

Thanks for your feedback. I actually used

govendor.exe fetch github.com/prometheus/client_golang/prometheus/...

to update all referenced packages under the prometheus path.

What started as an small prometheus API update ended in an excursion to the go package dependency nightmare 😨

😆

@martinlindhe
Copy link
Collaborator

martinlindhe commented Apr 30, 2017

Thanks for your work, looks good!

@martinlindhe martinlindhe merged commit 6bb522b into prometheus-community:master Apr 30, 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