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

fix: prometheus counters can only count up #561

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Nov 21, 2019

Which problem is this PR solving?

Short description of the changes

  • Export monotonic flag with metrics
  • Map non-monotonic counter to gauge in prometheus exporter

@codecov-io
Copy link

codecov-io commented Nov 21, 2019

Codecov Report

Merging #561 into master will decrease coverage by 1.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #561      +/-   ##
==========================================
- Coverage   92.58%   91.55%   -1.03%     
==========================================
  Files         158      133      -25     
  Lines        8022     6064    -1958     
  Branches      675      463     -212     
==========================================
- Hits         7427     5552    -1875     
+ Misses        595      512      -83
Impacted Files Coverage Δ
packages/opentelemetry-metrics/src/types.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-metrics/test/Meter.test.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-metrics/src/Metric.ts 89.09% <ø> (ø) ⬆️
packages/opentelemetry-metrics/src/export/types.ts 100% <ø> (ø) ⬆️
...pentelemetry-exporter-prometheus/src/prometheus.ts 92% <100%> (+0.16%) ⬆️
...emetry-exporter-prometheus/test/prometheus.test.ts 98.61% <100%> (+0.06%) ⬆️
...kages/opentelemetry-plugin-dns/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...opentelemetry-base/test/resources/resource.test.ts 100% <0%> (ø) ⬆️
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100% <0%> (ø) ⬆️
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts 100% <0%> (ø) ⬆️
... and 29 more

@mayurkale22
Copy link
Member

This approach seems simple and straightforward, but I am a little worried about changes to MetricDescriptor interface (which is exact replica of actual proto).

@dyladan
Copy link
Member Author

dyladan commented Nov 21, 2019

@mayurkale22 do you have an alternative suggestion? One thing I can think of is to do this at the SDK level (create a 'counter-like' wrapper around a gauge object if a non-monotonic counter is created), but this seems like confusing behavior if a metrics backend treats gauges and counters differently.

edit: another SDK level possibility is to create a CounterMetric that has _type = MetricDescriptorType.GAUGE_DOUBLE or _type = MetricDescriptorType.GAUGE_INT64

@dyladan
Copy link
Member Author

dyladan commented Nov 22, 2019

@mayurkale22 is there some reason the internal type has to exactly match the Proto? or is that just a convenience?

@dyladan
Copy link
Member Author

dyladan commented Nov 25, 2019

Let's talk about this at the SIG. I'll make a short doc explaining the situation to everyone.

@dyladan
Copy link
Member Author

dyladan commented Nov 27, 2019

Per SIG, we are going to send monotonic flag in the export type. We are only going to change the export type from the OT type when it needs to happen (non-monotonic counter exported as gauge).

@dyladan dyladan changed the title fix: prometheus counters can only count up #553 fix: prometheus counters can only count up Nov 27, 2019
@dyladan
Copy link
Member Author

dyladan commented Nov 27, 2019

@open-telemetry/javascript-approvers please give this a look

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, we need to do same thing for Monotonic Gauge based on open-telemetry/opentelemetry-specification#259 decision.

packages/opentelemetry-metrics/src/export/types.ts Outdated Show resolved Hide resolved
@dyladan
Copy link
Member Author

dyladan commented Nov 27, 2019

Overall LGTM, we need to do same thing for Monotonic Gauge based on open-telemetry/opentelemetry-specification#259 decision.

I'm not sure we should do that yet since the issue is not settled.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

examples/prometheus/index.js Outdated Show resolved Hide resolved
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

LGTM

@mayurkale22 mayurkale22 added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Dec 1, 2019
@mayurkale22 mayurkale22 merged commit 2e4136c into open-telemetry:master Dec 2, 2019
@dyladan dyladan deleted the prometheus-metric-compat-only branch December 2, 2019 17:03
@dyladan dyladan mentioned this pull request Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants