-
Notifications
You must be signed in to change notification settings - Fork 417
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
[EXPORTER] Export resource for prometheus #2301
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2301 +/- ##
==========================================
+ Coverage 87.39% 87.41% +0.02%
==========================================
Files 199 199
Lines 6009 6018 +9
==========================================
+ Hits 5251 5260 +9
Misses 758 758
|
exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
Outdated
Show resolved
Hide resolved
exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
Outdated
Show resolved
Hide resolved
Going through this link: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#resource-attributes-1, it states:
It states that resource attributes should be converted into a separate |
Sorry, I had some misunderstanding before. Could you please reviews the codes again and please let me know if there is any problem. |
exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
Outdated
Show resolved
Hide resolved
Signed-off-by: WenTao Ou <admin@owent.net>
Signed-off-by: WenTao Ou <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can open a follow-up issue to cache target_info so it doesn't need to be recomputed each scrape.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done. Thanks for the PR.
@@ -147,10 +170,10 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT | |||
} | |||
else | |||
{ | |||
sum = nostd::get<int64_t>(histogram_point_data.sum_); | |||
sum = static_cast<double>(nostd::get<int64_t>(histogram_point_data.sum_)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, but there is possibility of precision loss while casting from int64_t
to double
for larger values ( > 2^52) of sum
. Good to have a issue to track this (I will create one) - to check for any such loss and log a warning. No changes required in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I add this static cast because it will trigger a warning with clang 16 and -Werror
will make it a error.
BTW:Do you think think it may have epsilon problem in the code metric->histogram.sample_count = static_cast<std::uint64_t>(values[1]);
in PrometheusExporterUtils::SetValue
?
@owent Can you please resolve the conflict, probably one last change to merge this :) |
Signed-off-by: owent <admin@owent.net>
I'm on holiday these days. sorry for the delay. |
Signed-off-by: owent <admin@owent.net>
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small question on label reserve(size) or size +2.
Signed-off-by: owent <admin@owent.net>
There was a problem hiding this 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.
Fixes #1842
Changes
Follow https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#resource-attributes-1
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes