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

Counter and UpDownCounter temporality should be CUMULATIVE #1383

Closed
ocelotl opened this issue Nov 16, 2020 · 6 comments · Fixed by #1384
Closed

Counter and UpDownCounter temporality should be CUMULATIVE #1383

ocelotl opened this issue Nov 16, 2020 · 6 comments · Fixed by #1384
Assignees
Labels
bug Something isn't working

Comments

@ocelotl
Copy link
Contributor

ocelotl commented Nov 16, 2020

The temporalities specified here and here should be CUMULATIVE instead of DELTA.

@ocelotl ocelotl added the bug Something isn't working label Nov 16, 2020
@ocelotl ocelotl self-assigned this Nov 16, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Nov 16, 2020
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Nov 16, 2020
@jmacd
Copy link

jmacd commented Nov 16, 2020

This makes sense to me. The reason these should be CUMULATIVE here and now, in the present code base, is that the file opentelemetry/sdk/metrics/export/processor.py converts the input measurements (which are deltas, from these two instrument kinds) into the output aggregations (which are cumulatives) using a call like batch_value.merge(aggregator).

@aabmass
Copy link
Member

aabmass commented Nov 17, 2020

In the current metrics implementation, it will be DELTA if the stateful is false, so I think the temporality needs to be derived from stateful (related #1007). The code Josh mentioned resets the batch map in when not stateful, so the merging doesn't happen and you get DELTAs:

def finished_collection(self):
"""Performs certain post-export logic.
For processors that are stateless, resets the batch map.
"""
if not self.stateful:
self._batch_map = {}

batch_value = self._batch_map.get(key)
if batch_value:
# Update the stored checkpointed value if exists. The call to merge
# here combines only identical records (same key).
batch_value.merge(aggregator)

@aabmass
Copy link
Member

aabmass commented Nov 17, 2020

IIRC, it should be configurable. In Go, it defaults to cumulative but can be configured to delta, kind of like stateful.

@lzchen
Copy link
Contributor

lzchen commented Nov 18, 2020

@aabmass @jmacd

We currently have stateful to be configurable. I believe Go has implemented in such a way that the pipeline decides on whether a metric is stateful or not? stateful is also not mentioned anywhere in the sdk spec, so I don't know if we want to depend on something that potentially might not exist?

ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue Nov 18, 2020
@ocelotl
Copy link
Contributor Author

ocelotl commented Nov 18, 2020

This PR fixes the aggregation temporality for the Counter and UpDownCounter instruments which is an improvement, @aabmass, maybe this can be merged, and the changes related to stateful can be fixed in the PR for #1007?

@aabmass
Copy link
Member

aabmass commented Nov 18, 2020

@ocelotl SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants