Skip to content

Santize utf8 label values #19

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

Closed
wants to merge 5 commits into from

Conversation

tivvit
Copy link
Contributor

@tivvit tivvit commented May 20, 2020

using #18

based on alicebob/asprom#42 - there is a description of the problem and the used solution is very similar - in this project, there is no wrapper function for metrics so I had to add it to more places.

@khaf
Copy link
Contributor

khaf commented May 21, 2020

Can we please have this PR decoupled from the #18? At this moment that changeset cannot not be merged.

@tivvit
Copy link
Contributor Author

tivvit commented May 21, 2020

Yes I will prepare it. I am used to using modules, therefore it was more comfortable for me to do it like this, sorry for that

@tivvit
Copy link
Contributor Author

tivvit commented May 21, 2020

done @khaf

@khaf
Copy link
Contributor

khaf commented May 21, 2020

Thank you, this is now much more clear to me. But I think there is no reason to pepper UTF8 validation all over the codebase.

There is a central function on the Observer that sends the requests to the server node and gets the result back. Those results are then sent to watchers to be parsed and used. That function stands in for the wrapper you are looking for.

You can add validations only in Observer.requestInfo#L205 by ranging over rawMetrics map and validating the values:

for range k, v := range rawMetrics {
  rawMetrics[k] = sanitizeLabelValue(v)
}

Is that what are looking for?

@tivvit
Copy link
Contributor Author

tivvit commented May 21, 2020

thanks for the tip, this your proposed solution is much better

@khaf
Copy link
Contributor

khaf commented May 22, 2020

OK I merged this PR, but ended up rebasing it due to conflict with the other PR. Thank you for your efforts.

@khaf khaf closed this May 22, 2020
@tivvit tivvit deleted the santize-utf8-label-values branch May 22, 2020 13:21
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.

2 participants