Skip to content

Fix Prometheus exporter failure#1460

Merged
esigo merged 3 commits intoopen-telemetry:mainfrom
esigo:prometheus-exporter-failure
Jun 22, 2022
Merged

Fix Prometheus exporter failure#1460
esigo merged 3 commits intoopen-telemetry:mainfrom
esigo:prometheus-exporter-failure

Conversation

@esigo
Copy link
Copy Markdown
Member

@esigo esigo commented Jun 21, 2022

Fixes histogram_point_data.sum_ variant access, also limits the max_collection_size of Prometheus collector needed for unit test.

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@esigo esigo requested a review from a team June 21, 2022 14:33
@esigo esigo added area:exporter:prometheus Prometheus Exporter bug Something isn't working labels Jun 21, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 21, 2022

Codecov Report

Merging #1460 (0c9c200) into main (96534a7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1460   +/-   ##
=======================================
  Coverage   85.30%   85.30%           
=======================================
  Files         154      154           
  Lines        4373     4373           
=======================================
  Hits         3730     3730           
  Misses        643      643           

{
sum = nostd::get<long>(histogram_point_data.sum_);
}
SetData(std::vector<double>{sum, (double)histogram_point_data.count_}, boundaries,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit - Why do we have std::vector<double> as first argument. Will we need to pass vector of aggregated values in this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That came from the legacy exporter code that I reused. The vector is used in SetValue called by SetData, supposedly to have slightly similar overload signature.

Copy link
Copy Markdown
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

@esigo esigo merged commit 5ccca49 into open-telemetry:main Jun 22, 2022
@esigo esigo deleted the prometheus-exporter-failure branch June 22, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:exporter:prometheus Prometheus Exporter bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants