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

feat(prometheus): update prometheus exporter with wip metrics sdk #2824

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 9, 2022

Which problem is this PR solving?

Fixes Partially #2773

This PR updates the Prometheus Exporter to the latest Metrics SDK type definitions.

  • We may need to split the necessary part of changes on sdk-metrics-base into separated PRs.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • PrometheusSerializer
  • PrometheusExporter

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Mar 9, 2022

Codecov Report

Merging #2824 (f86947d) into main (da22673) will increase coverage by 0.40%.
The diff coverage is n/a.

❗ Current head f86947d differs from pull request most recent head 15b793b. Consider uploading reports for the commit 15b793b to get more accurate results

@@            Coverage Diff             @@
##             main    #2824      +/-   ##
==========================================
+ Coverage   93.06%   93.46%   +0.40%     
==========================================
  Files         150      162      +12     
  Lines        4713     5568     +855     
  Branches      994     1178     +184     
==========================================
+ Hits         4386     5204     +818     
- Misses        327      364      +37     
Impacted Files Coverage Δ
...exporter-metrics-otlp-http/src/transformMetrics.ts 95.65% <0.00%> (ø)
packages/exporter-trace-otlp-http/src/util.ts 100.00% <0.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 97.00% <0.00%> (ø)
...s/exporter-trace-otlp-http/src/OTLPExporterBase.ts 100.00% <0.00%> (ø)
...ry-exporter-prometheus/src/PrometheusSerializer.ts 95.12% <0.00%> (ø)
...ation-xml-http-request/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...etry-exporter-prometheus/src/PrometheusExporter.ts 93.42% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...mentation-xml-http-request/src/enums/EventNames.ts 100.00% <0.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts 97.57% <0.00%> (ø)
... and 2 more

@legendecas legendecas force-pushed the metrics-ff/prometheus branch 2 times, most recently from b7f763d to 675f7e2 Compare March 23, 2022 02:37
@legendecas legendecas force-pushed the metrics-ff/prometheus branch from 675f7e2 to fbea6b9 Compare March 23, 2022 02:50
@legendecas legendecas marked this pull request as ready for review March 23, 2022 03:41
@legendecas legendecas requested a review from a team March 23, 2022 03:41
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

I didn't see anything in the code, but do you do anything with unit? In the python exporter, we use the official prometheus client library to generate text format and it actually accepts the unit and concatenates it into the metric name. Example test case.

I know there is a current spec effort to document OTLP<->Prometheus conversion, but it doesn't say anything about unit right now. Thoughts?

Update: found this in the prometheus docs. I will check with my coworker who's working on the OTLP to prom spec to see if this should be officially added.

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.

LGTM it's really a lot less change than I thought would be needed.

@@ -1,6 +1,7 @@
{
"name": "@opentelemetry/exporter-prometheus",
"name": "@opentelemetry/exporter-prometheus-wip",
Copy link
Member

Choose a reason for hiding this comment

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

After the OTLP exporters are updated we should remove wip from all names and have a release

Copy link
Member

Choose a reason for hiding this comment

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

No need to change the name here I think. The package is private and nothing depends on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted :)

@@ -1,6 +1,7 @@
{
"name": "@opentelemetry/exporter-prometheus",
"name": "@opentelemetry/exporter-prometheus-wip",
Copy link
Member

Choose a reason for hiding this comment

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

No need to change the name here I think. The package is private and nothing depends on it.

@legendecas legendecas merged commit b749b0d into open-telemetry:main Mar 28, 2022
@legendecas legendecas deleted the metrics-ff/prometheus branch March 28, 2022 14:52
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.

4 participants