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.

Sign up for GitHub

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

Jump to bottom

Avoid generator in _ViewInstrumentMatch.collect() #3035

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Avoid generator in _ViewInstrumentMatch.collect() #3035

merged 1 commit into from
Nov 22, 2022

Conversation

lancetarn
Copy link
Contributor

@lancetarn lancetarn commented Nov 15, 2022 •
edited
Loading

Description

Alter _ViewInstrumentMatch.collect() to return a realized List of data_points rather than a Generator. This probably has some memory implications, but I don't have a good handle on how impactful they are likely to be.

Also updates CONTRIBUTING.md to remove mention of metrics branch.

Fixes #3021

Type of change

  • Bug fix (non-breaking change which fixes an issue)

This does change a return type, but it appears effectively internal and nothing within the SDK seemed to attempt to use the result in an incompatible way. The tests were actually assuming it was an Iterator rather than just an Iterable, I believe, so updating them felt like a fix.

How Has This Been Tested?

Installed in editable mode with 3.9 and an external script to send signals to the console and a remote collector. Not a load test, but everything was collected and sent as expected.

Does This PR Require a Contrib Repo Change?

I did remove reference to the metrics branch in CONTRIBUTING.md, but that does not seem relevant.

  • No.

Checklist:

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

@lancetarn lancetarn requested a review from a team November 15, 2022 16:33
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 15, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@lancetarn lancetarn changed the title Avoid generator in _ViewInstrumentMatch.collect() [WIP] Avoid generator in _ViewInstrumentMatch.collect() Nov 15, 2022
@lancetarn lancetarn changed the title [WIP] Avoid generator in _ViewInstrumentMatch.collect() Avoid generator in _ViewInstrumentMatch.collect() Nov 15, 2022
aabmass
aabmass approved these changes Nov 16, 2022
View reviewed changes
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.

Thanks for the PR! This will definitely use more memory but I think that's unavoidable

@ocelotl
Copy link
Contributor

ocelotl commented Nov 16, 2022

This could be a bug fix, @lancetarn, please add a CHANGELOG entry. ✌️

@lancetarn
Copy link
Contributor Author

This could be a bug fix, @lancetarn, please add a CHANGELOG entry. ✌️

CHANGELOG updated! Thanks @ocelotl You (or someone) might need to kick off CI again for it.

@lancetarn lancetarn force-pushed the 3021-metrics-iterator-to-sequence branch from 0a930e8 to 10db92b Compare November 16, 2022 19:22
@lancetarn
Copy link
Contributor Author

It looks like one of the tornado contrib tests assumes data_points is an Iterator as well. Should I PR there to first change the test implementation, then re-run these?

I've reproduced this by checkout out the contrib repo and pointing CORE_REPO at my forked version; if there is a better way I'm interested but that seems ok.

This was referenced Nov 17, 2022
Closed
Merged
@lancetarn
Copy link
Contributor Author

Okay I have set open-telemetry/opentelemetry-python-contrib#1456 up to solve the problem. Hopefully that was a reasonable thing to do.

@srikanthccv
Copy link
Member

You will also have to update the Contrib SHA here with last commit hash that fixed the issue on contrib repo https://github.com/open-telemetry/opentelemetry-python/blob/main/.github/workflows/test.yml#L13

@lancetarn
Copy link
Contributor Author

You will also have to update the Contrib SHA here with last commit hash that fixed the issue on contrib repo https://github.com/open-telemetry/opentelemetry-python/blob/main/.github/workflows/test.yml#L13

Would this later just have to be reverted? I was intending to just let the fix land over there first, then re-run these.

@lzchen
Copy link
Contributor

lzchen commented Nov 21, 2022

@lancetarn
I believe the SHA would still have to be updated since that commit is what the contrib repo tests for THIS repo will be run off of.

@lancetarn lancetarn force-pushed the 3021-metrics-iterator-to-sequence branch from 10db92b to 1e49add Compare November 22, 2022 15:45
@lancetarn
Avoid generator in _ViewInstrumentMatch.collect()
2be1687 
- Fix open-telemetry#3021
- Remove metric branch reference from CONTRIBUTING.md
@lancetarn lancetarn force-pushed the 3021-metrics-iterator-to-sequence branch from 1e49add to 2be1687 Compare November 22, 2022 15:47
@lancetarn
Copy link
Contributor Author

@lzchen Now I see, thank you. I'll point it back at main for now. Sorry for all the back-and-forth, but I have a better understanding of the mechanics of these repos now.

@lzchen
Copy link
Contributor

lzchen commented Nov 22, 2022

@lancetarn
Yeah the structure is a bit confusing. Thanks for your patience!

@lzchen lzchen merged commit 5574f7e into open-telemetry:main Nov 22, 2022
shalevr added a commit to shalevr/opentelemetry-python that referenced this pull request Nov 28, 2022
@shalevr
Merge branch 'decode-values-from-otel-resource-attributes' of https:/…
3e02f52 
…/github.com/shalevr/opentelemetry-python into decode-values-from-otel-resource-attributes

* 'decode-values-from-otel-resource-attributes' of https://github.com/shalevr/opentelemetry-python:
  Avoid generator in _ViewInstrumentMatch.collect() (open-telemetry#3035)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@aabmass aabmass aabmass approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sum/Histogram/Gauge.data_points annotated as Sequence but actual type is generator
5 participants
@lancetarn @ocelotl @srikanthccv @lzchen @aabmass