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

Merge Asynchronous and Synchronous sum aggregations #2379

Merged
merged 13 commits into from
Feb 11, 2022

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jan 15, 2022

Fixes #2353

@ocelotl ocelotl added metrics Skip Changelog PRs that do not require a CHANGELOG.md entry labels Jan 15, 2022
@ocelotl ocelotl requested a review from a team January 15, 2022 01:45
@ocelotl ocelotl self-assigned this Jan 15, 2022
@ocelotl ocelotl requested a review from aabmass January 15, 2022 01:46
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 know in my original prototype and doc, we had collect() -> Optional[Point] so that "the aggregations may also return None if no measurements were seen in this interval–the metric reader won't see see the point in this case." I remember there being some specification discussion around whether or not this is correct, but can't dig anything up.

Given the confusion and fact that this would be an optimization, should put that off till later and just do the simple thing to start?

@ocelotl
Copy link
Contributor Author

ocelotl commented Jan 19, 2022

I know in my original prototype and doc, we had collect() -> Optional[Point] so that "the aggregations may also return None if no measurements were seen in this interval–the metric reader won't see see the point in this case." I remember there being some specification discussion around whether or not this is correct, but can't dig anything up.

Given the confusion and fact that this would be an optimization, should put that off till later and just do the simple thing to start?

All right, removed any association between self._value and None 😆

@ocelotl ocelotl requested a review from aabmass January 19, 2022 17:55
@ocelotl ocelotl added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Jan 24, 2022
@ocelotl ocelotl requested review from lzchen and srikanthccv January 26, 2022 00:19
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Please address conflicts.

@ocelotl
Copy link
Contributor Author

ocelotl commented Feb 9, 2022

Please address conflicts.

Done, @aabmass plese review ✌️

This is done in order to identify the situation when aggregate has not
been called before collect is.
@ocelotl ocelotl merged commit ec3053e into open-telemetry:main Feb 11, 2022
@ocelotl ocelotl deleted the issue_2353 branch February 11, 2022 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary metrics Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Async aggregations
5 participants