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.

Sign up for GitHub

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

Jump to bottom

Cleanup Exporter a bit #22

Merged
merged 2 commits into from
Apr 12, 2022
Merged

Cleanup Exporter a bit #22

merged 2 commits into from
Apr 12, 2022

Conversation

dmke
Copy link
Contributor

@dmke dmke commented Apr 1, 2022 •
edited
Loading

This makes some minor adjustments:

  • Exporter gains a WithInterfaces configuration knob.
  • Reduce unnecessary allocation in intoDesc.
  • API tokens can be configured via environment.

The last item is useful when running the Docker container: Instead of mounting a file and adding a CLI flag, one can just manage the environment:

$ # before
$ cat ./config.yml
token:
  unms.example.com: a-uuid-token
$ docker run --rm -it -p 9806:9806 -v $(pwd)/config.yml:/config.yml:ro quay.io/ffddorf/unms-exporter /usr/local/bin/unms-exporter -c /config.yml

$ # after
$ docker run --rm -it -p 9806:9806 -e UNMS_EXPORTER_TOKEN="unms.example.com=a-uuid-token" quay.io/ffddorf/unms-exporter

$ # more secure (credentials are not exposed in procfs):
$ export UNMS_EXPORTER_TOKEN="unms.example.com=a-uuid-token"
$ docker run --rm -it -p 9806:9806 -e UNMS_EXPORTER_TOKEN quay.io/ffddorf/unms-exporter

@dmke dmke mentioned this pull request Apr 1, 2022
Merged
@mraerino
Copy link
Member

mraerino commented Apr 5, 2022

hey @dmke, thanks for all your contributions. I'm going to get to them this weekend hopefully.

@dmke
Copy link
Contributor Author

dmke commented Apr 5, 2022

In #23, it looks like I've got some units wrong, let me fix that real quick :)

image

mraerino
mraerino reviewed Apr 8, 2022
View reviewed changes
Copy link
Member

@mraerino mraerino left a comment •
edited
Loading

Choose a reason for hiding this comment

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

the cleanup bits look nice, the new behaviour is a bit confusing to me

exporter/exporter.go Outdated Show resolved Hide resolved
exporter/device.go Show resolved Hide resolved
exporter/device.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@dmke dmke force-pushed the chore/cleanup branch 5 times, most recently from 0dc2bf7 to 2bc5d73 Compare April 8, 2022 18:43
@dmke
Copy link
Contributor Author

dmke commented Apr 8, 2022

I believe this PR is now ready for the next review round.

mraerino
mraerino reviewed Apr 9, 2022
View reviewed changes
Copy link
Member

@mraerino mraerino left a comment •
edited
Loading

Choose a reason for hiding this comment

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

hmm, your changes are still confusing.

so the switch is now called wan_metrics but it disables metrics collection for all interfaces? that doesn't make sense to me

exporter/device.go Outdated Show resolved Hide resolved
exporter/device.go Outdated Show resolved Hide resolved
@mraerino
Copy link
Member

mraerino commented Apr 9, 2022

let's try to break this PR up.
there is clearly some useful cleanup work in this, but the change about omitting interface statistics should probably be broken out and discussed separately.

@dmke
Copy link
Contributor Author

dmke commented Apr 9, 2022

Fair enough. That will take a moment, though.

@dmke
Copy link
Contributor Author

dmke commented Apr 9, 2022

Done.

@dmke dmke mentioned this pull request Apr 9, 2022
Closed
mraerino
mraerino reviewed Apr 11, 2022
View reviewed changes
Copy link
Member

@mraerino mraerino 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 thing

exporter/exporter.go Outdated
var (
defaultWithInterfaces = true
)
var defaultWithInterfaces = true
Copy link
Member

Choose a reason for hiding this comment

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

mind moving this to device.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@mraerino
Copy link
Member

please consider doing a merge instead of a rebase (github even has a button for it)
since we'll squash the PR, the history only matters for reviewers who will have a much easier time by being able to see the clean merges

@dmke
Copy link
Contributor Author

dmke commented Apr 11, 2022

Oh, sorry, force of habit :)

@dmke
Cleanup Exporter a bit
335571a 
- move unwieldy prom.MustNewConstMetric into a helper func
- move fetching UNMS data into a helper func
- reduce unnecessay allocation in intoDesc
- make API tokens configurable via environment
- run `go mod tidy`
mraerino
mraerino reviewed Apr 11, 2022
View reviewed changes
Copy link
Member

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

sorry, i keep finding things 🙈

exporter/device.go Show resolved Hide resolved
mraerino
mraerino approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

thank you. these are great!

@mraerino mraerino merged commit 58dded0 into ffddorf:main Apr 12, 2022
@dmke dmke deleted the chore/cleanup branch April 14, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mraerino mraerino mraerino approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dmke @mraerino