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

fix: ensure the label names are sanitised #2121

Merged
merged 8 commits into from Apr 21, 2021

Conversation

weyert
Copy link
Contributor

@weyert weyert commented Apr 19, 2021

Sanitises the attribute names to match the naming convention enforced by
Prometheus

Which problem is this PR solving?

If you have a label name with - then Prometheus will fall over this issue with an error like:
error while linting: text format parsing error in line 282: expected '=' after label name, found '-'

Short description of the changes

The change is to also sanitise the label name and not only the metric names

Sanitises the attribute names to match the naming convention enforced by
Prometheus
@weyert
Copy link
Contributor Author

weyert commented Apr 19, 2021

Alternative way to fix this is to apply this check and when the diag level is DEBUG output a warning that the attribute is not meeting Prometheus requirements

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

I think sanitizing is fine. Better than dropping at least. We could add a debug log but it would likely be quite chatty. We could add each bad label name to a Set of names that have already been logged. Only log each bad name once.

Approving for now since I see no immediate concern.

@dyladan dyladan added the bug Something isn't working label Apr 19, 2021
@dyladan
Copy link
Member

dyladan commented Apr 19, 2021

Looks like your time mock failed

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #2121 (41a7522) into main (c516264) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 41a7522 differs from pull request most recent head 952f86f. Consider uploading reports for the commit 952f86f to get more accurate results

@@            Coverage Diff             @@
##             main    #2121      +/-   ##
==========================================
- Coverage   92.78%   92.74%   -0.04%     
==========================================
  Files         140      140              
  Lines        4976     4979       +3     
  Branches     1028     1028              
==========================================
+ Hits         4617     4618       +1     
- Misses        359      361       +2     
Impacted Files Coverage Δ
...ry-exporter-prometheus/src/PrometheusSerializer.ts 98.07% <100.00%> (+0.05%) ⬆️
...ges/opentelemetry-instrumentation-http/src/http.ts 94.82% <0.00%> (-0.80%) ⬇️

@weyert
Copy link
Contributor Author

weyert commented Apr 19, 2021

I think sanitizing is fine. Better than dropping at least. We could add a debug log but it would likely be quite chatty. We could add each bad label name to a Set of names that have already been logged. Only log each bad name once.

Approving for now since I see no immediate concern.

Yeah, that could work. I am happy to make a change later :)

@dyladan
Copy link
Member

dyladan commented Apr 19, 2021

This likely won't be merged right away to ensure there is time for people to see it before merged but if it isn't merged by tomorrow I will merge it. If you have time to make the logging change before merge then great but if not it isn't blocking and can be done as follow up.

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
@vmarchaud vmarchaud requested a review from obecny April 20, 2021 15:44
@obecny
Copy link
Member

obecny commented Apr 20, 2021

@weyert you have typo in variable name santizedLabelName should be sanitizedLabelName you are missing i

@dyladan
Copy link
Member

dyladan commented Apr 21, 2021

@obecny I fixed the typo. I want to merge this for 0.19 release if possible

@dyladan dyladan merged commit 4f9d2b2 into open-telemetry:main Apr 21, 2021
@dyladan dyladan changed the title feat: ensure the label names are sanitised fix: ensure the label names are sanitised Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants