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

[prometheusremotewrite] fix: counter name check #2613

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented Mar 6, 2021

fixes #2608

Ensure that the metric name is greater than the length of the
counter suffix ("_total") before checking if it contains the counter
suffix to prevent crashes for short (smaller or eq to the length of the
suffix) metric names.

The edge case where the metric name is "_total" is not handled (considered
not to have the suffix), but should not occur IRL.

edit: Use strings.hasSuffix() to check if metric name has the counter suffix ("_total")

Also fix the spelling of "delimiter".

warning: this is one of my first PRs in Go 😅

@naseemkullah naseemkullah requested a review from a team March 6, 2021 19:32
@naseemkullah naseemkullah changed the title Fix prom remote write counter name check [prometheusremotewrite] fix: counter name check Mar 6, 2021
fixes  open-telemetry#2608

Ensure that the metric name is greater than the length of the
counter suffix ("_total") before checking if it contains the counter
suffix to prevent crashes for short (smaller or eq to the length of the
suffix) metric names.

The edge case where the metric name is "_total" is not handled (considered
not to have the suffix), but should not occur IRL.

Also fix the spelling of "delimiter".

Signed-off-by: naseemkullah <naseem@transit.app>
@naseemkullah naseemkullah force-pushed the fix-prom-remote-write-counter-name-check branch from 42bb509 to 8bffb8e Compare March 6, 2021 19:52
@codecov
Copy link

codecov bot commented Mar 6, 2021

Codecov Report

Merging #2613 (3bd2bd7) into main (2f38767) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2613      +/-   ##
==========================================
- Coverage   92.03%   92.03%   -0.01%     
==========================================
  Files         273      273              
  Lines       15452    15450       -2     
==========================================
- Hits        14221    14219       -2     
- Misses        848      849       +1     
+ Partials      383      382       -1     
Impacted Files Coverage Δ
exporter/prometheusremotewriteexporter/helper.go 99.54% <100.00%> (-0.01%) ⬇️
testutil/testutil.go 81.60% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f38767...3bd2bd7. Read the comment docs.

Signed-off-by: naseemkullah <naseem@transit.app>
@naseemkullah naseemkullah requested a review from bogdandrutu March 6, 2021 20:47
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Would be good to have a test :)

Signed-off-by: naseemkullah <naseem@transit.app>
@naseemkullah naseemkullah force-pushed the fix-prom-remote-write-counter-name-check branch from 69e31e7 to 3bd2bd7 Compare March 8, 2021 06:19
@naseemkullah
Copy link
Member Author

Would be good to have a test :)

done, PTAL -> 3bd2bd7

@bogdandrutu bogdandrutu merged commit 35cf41e into open-telemetry:main Mar 8, 2021
@naseemkullah naseemkullah deleted the fix-prom-remote-write-counter-name-check branch March 8, 2021 17:57
This was referenced Mar 11, 2021
This was referenced Mar 15, 2021
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
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.

prometheusremotewrite exporter crashes collector with metrics names < 6 characters
2 participants