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

[EXPORTER] Export resource for prometheus #2301

Merged
merged 34 commits into from
Oct 3, 2023
Merged

Conversation

owent
Copy link
Member

@owent owent commented Sep 8, 2023

Fixes #1842

Changes

  • Prometheus exporter emits resource attributes
  • Fix incorrect address point in unit test of prometheus utils.

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
  • Unit tests have been added
  • Changes in public API reviewed

@owent owent requested a review from a team September 8, 2023 16:27
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #2301 (0149d5c) into main (0803e6a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Files Coverage Δ
...opentelemetry/sdk/metrics/export/metric_producer.h 100.00% <100.00%> (ø)

@owent owent changed the title [WIP] Export resource for prometheus Export resource for prometheus Sep 9, 2023
@owent owent requested a review from dbolduc September 13, 2023 02:12
@lalitb
Copy link
Member

lalitb commented Sep 13, 2023

Going through this link: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#resource-attributes-1, it states:

In SDK Prometheus (pull) exporters, resource attributes SHOULD be converted to a single [target_info metric](https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#supporting-target-metadata-in-both-push-based-and-pull-based-systems); otherwise, they MUST be dropped, and MUST NOT be attached as labels to other metric families. The target_info metric MUST be an info-typed metric whose labels MUST include the resource attributes, and MUST NOT include any other labels. There MUST be at most one target_info metric exposed on an SDK Prometheus endpoint.

It states that resource attributes should be converted into a separate target_info metrics, or else dropped. I don't see we are doing it here.

@owent
Copy link
Member Author

owent commented Sep 14, 2023

Going through this link: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#resource-attributes-1, it states:

In SDK Prometheus (pull) exporters, resource attributes SHOULD be converted to a single [target_info metric](https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#supporting-target-metadata-in-both-push-based-and-pull-based-systems); otherwise, they MUST be dropped, and MUST NOT be attached as labels to other metric families. The target_info metric MUST be an info-typed metric whose labels MUST include the resource attributes, and MUST NOT include any other labels. There MUST be at most one target_info metric exposed on an SDK Prometheus endpoint.

It states that resource attributes should be converted into a separate target_info metrics, or else dropped. I don't see we are doing it here.

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/src/exporter_utils.cc Outdated Show resolved Hide resolved
exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
exporters/prometheus/src/collector.cc Outdated Show resolved Hide resolved
exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@dashpole dashpole left a 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.

exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
Copy link
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.

Nicely done. Thanks for the PR.

exporters/prometheus/src/collector.cc Outdated Show resolved Hide resolved
@@ -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_));
Copy link
Member

@lalitb lalitb Oct 2, 2023

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.

Copy link
Member Author

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 ?

@lalitb
Copy link
Member

lalitb commented Oct 2, 2023

@owent Can you please resolve the conflict, probably one last change to merge this :)

@owent
Copy link
Member Author

owent commented Oct 3, 2023

I'm on holiday these days. sorry for the delay.

Signed-off-by: owent <admin@owent.net>
@owent
Copy link
Member Author

owent commented Oct 3, 2023

@owent Can you please resolve the conflict, probably one last change to merge this :)

Done

Copy link
Member

@marcalff marcalff left a 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>
Copy link
Member

@marcalff marcalff 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.

@marcalff marcalff merged commit 0eaa794 into open-telemetry:main Oct 3, 2023
45 checks passed
@marcalff marcalff changed the title Export resource for prometheus [EXPORTER] Export resource for prometheus Oct 11, 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.

Prometheus exporter emits target_info metric using Resource
6 participants