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

prom: Remove custom registry #3161

Merged
merged 3 commits into from
Sep 18, 2019

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Sep 17, 2019

This change renames namespaces and removes the elem_id label from all metrics.
The element ID configured in the config is exposed through scion_elem_id.

namespace renames:

  • beacon_srv -> bs
  • border -> br
  • cert_srv -> cs
  • dispatcher -> disp
  • path_srv -> ps
  • sciond -> sd

fixes #3160


This change is Reviewable

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 34 of 34 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @oncilla)

a discussion (no related file):
I think this is a partly breaking change. I would add the breaking change label. Also not in the desription that metrics will be renamed, optimally with a list of old namespace -> new_namespace.



go/cert_srv/internal/metrics/metrics.go, line 18 at r1 (raw file):

n

Also make it public?


go/godispatcher/internal/metrics/metrics.go, line 26 at r1 (raw file):

dispatcher

maybe disp?


go/lib/prom/prom.go, line 77 at r1 (raw file):

// Note this should be called before any other interaction with prometheus.
// See also: https://github.com/prometheus/client_golang/issues/515
func UseDefaultRegWithElem(elemId string) {

Why is that still here?


go/sciond/internal/metrics/metrics.go, line 18 at r1 (raw file):

sciond

sd ?

@oncilla oncilla added the i/breaking change PR that breaks forwards or backwards compatibility label Sep 18, 2019
Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 20 of 36 files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker and @oncilla)


go/godispatcher/internal/metrics/metrics.go, line 26 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
dispatcher

maybe disp?

Done.


go/lib/prom/prom.go, line 77 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Why is that still here?

Forgot to remove.
Done.


go/sciond/internal/metrics/metrics.go, line 18 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
sciond

sd ?

Done.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 20 of 36 files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think this is a partly breaking change. I would add the breaking change label. Also not in the desription that metrics will be renamed, optimally with a list of old namespace -> new_namespace.

done


Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 20 of 36 files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker)


go/cert_srv/internal/metrics/metrics.go, line 18 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
n

Also make it public?

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 16 of 16 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@oncilla oncilla merged commit afb63c0 into scionproto:master Sep 18, 2019
@oncilla oncilla deleted the pub-metrics-remove-custom-registry branch September 18, 2019 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i/breaking change PR that breaks forwards or backwards compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prom: Get rid of custom registry
2 participants