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

promhttp: Check validity of method and code label values #962

Merged
merged 5 commits into from Jan 18, 2022

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Jan 17, 2022

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun requested review from beorn7 and bwplotka Jan 17, 2022
Copy link
Member

@bwplotka bwplotka left a comment

This is epic! Proposed a more flexible API, otherwise, it's perfect, thanks!

prometheus/promhttp/instrument_client.go Outdated Show resolved Hide resolved
prometheus/promhttp/instrument_client.go Outdated Show resolved Hide resolved
kakkoyun added 2 commits Jan 17, 2022
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Copy link
Member

@beorn7 beorn7 left a comment

LGTM in general, just a few thoughts.

prometheus/promhttp/instrument_server.go Outdated Show resolved Hide resolved
if len(additionalMethods) > 0 {
for _, method := range additionalMethods {
if strings.EqualFold(m, method) {
return strings.ToLower(m)
Copy link
Member

@beorn7 beorn7 Jan 17, 2022

Choose a reason for hiding this comment

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

Can't we just use method here instead of m? It wouldn't require ToLower if we documented that additional methods are used with the same capitalization as provided (which some users might even need).

Copy link
Member Author

@kakkoyun kakkoyun Jan 18, 2022

Choose a reason for hiding this comment

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

Of course, it's an option. I just respected the previous design decisions here. Everything was the lower case before and just kept it. I'm inclined to keep the lower case behaviour and if someone requests it we can change it; it's a minor one anyways.

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Copy link
Member

@bwplotka bwplotka left a comment

LGTM, just a small inconsistency. I would pick shorter name, additional is quite long word to use everywhere and in those case we often say extra, but up to you (:

if s >= 100 && s <= 599 {
return strconv.Itoa(s)
}
return "unknown"
Copy link
Member

@bwplotka bwplotka Jan 18, 2022

Choose a reason for hiding this comment

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

(:

prometheus/promhttp/option.go Outdated Show resolved Hide resolved
prometheus/promhttp/instrument_server.go Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun merged commit 9075cdf into main Jan 18, 2022
8 checks passed
@kakkoyun kakkoyun deleted the sanitize_method_and_code branch Jan 18, 2022
bwplotka pushed a commit that referenced this issue Feb 15, 2022
* Check validity of method and code label values

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Use more flexibly functional option pattern for configuration

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update documentation

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Simplify

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Fix inconsistent method naming

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
bwplotka added a commit that referenced this issue Feb 15, 2022
* Check validity of method and code label values

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Use more flexibly functional option pattern for configuration

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update documentation

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Simplify

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Fix inconsistent method naming

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

Co-authored-by: Kemal Akkoyun <kakkoyun@users.noreply.github.com>
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.

None yet

3 participants