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

chore(deps): support cumulative, delta, and pass-through exporters #2118

Merged

Conversation

sergeylanzman
Copy link
Contributor

@sergeylanzman sergeylanzman commented Apr 17, 2021

Which problem is this PR solving?

open-telemetry/opentelemetry-specification#731

Short description of the changes

Use AGGREGATION_TEMPORALITY_CUMULATIVE by default for all metrics and add option for pass AGGREGATION_TEMPORALITY on meter creating. Now counter meter can be work with js => collector => prometheus flow

@codecov
Copy link

codecov bot commented Apr 17, 2021

Codecov Report

Merging #2118 (bf0ce96) into main (d504f32) will increase coverage by 0.01%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main    #2118      +/-   ##
==========================================
+ Coverage   92.75%   92.77%   +0.01%     
==========================================
  Files         140      140              
  Lines        4998     4997       -1     
  Branches     1032     1028       -4     
==========================================
  Hits         4636     4636              
+ Misses        362      361       -1     
Impacted Files Coverage Δ
packages/opentelemetry-metrics/src/export/types.ts 100.00% <ø> (ø)
packages/opentelemetry-metrics/src/Metric.ts 90.24% <80.00%> (-1.43%) ⬇️
...ages/opentelemetry-api-metrics/src/types/Metric.ts 100.00% <100.00%> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts 95.71% <100.00%> (+0.71%) ⬆️
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

overall looks good, quite comment though

packages/opentelemetry-metrics/src/export/types.ts Outdated Show resolved Hide resolved
@sergeylanzman sergeylanzman force-pushed the support-cumulative-default-metrics branch from 8dfbabb to eb30485 Compare April 17, 2021 20:51
@obecny
Copy link
Member

obecny commented Apr 19, 2021

The mentioned issue is still opened for quite long time already. Does this mean the decision / spec is not yet final ?
I would like to avoid changing to something that will be then changed again and again - it was like this already couple times especially with the temporality and it was producing lots of confusion. I would wait until it is confirmed and merged to spec.

@sergeylanzman
Copy link
Contributor Author

The mentioned issue is still opened for quite long time already. Does this mean the decision / spec is not yet final ?
I would like to avoid changing to something that will be then changed again and again - it was like this already couple times especially with the temporality and it was producing lots of confusion. I would wait until it is confirmed and merged to spec.

Currently it's not working in JS lib. This code in collector drop every delta metric(counter) https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/prometheusexporter/accumulator.go#L238-L241

Go lib use same logic(AGGREGATION_TEMPORALITY_CUMULATIVE by default) and option for use delta.

This PR still provide option use AGGREGATION_TEMPORALITY_DELTA, but fix mainstream flow(JS=>collector=> prometheus).

Without this PR opentelemetry-js useless with collector and prometheus

@obecny
Copy link
Member

obecny commented Apr 21, 2021

FYI: need to check something, will get back to this tomorrow.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants