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

Sum/Histogram/Gauge.data_points annotated as Sequence but actual type is generator #3021

Closed
aabmass opened this issue Nov 4, 2022 · 2 comments · Fixed by #3035
Closed

Sum/Histogram/Gauge.data_points annotated as Sequence but actual type is generator #3021

aabmass opened this issue Nov 4, 2022 · 2 comments · Fixed by #3035
Assignees
Labels
bug Something isn't working metrics sdk Affects the SDK package.

Comments

@aabmass
Copy link
Member

aabmass commented Nov 4, 2022

This applies to all of the MetricsData data types Sum, Histogram and Gauge. The type annotation is Sequence:

But the implementation assigns an Iterable (specifically a generator which is not a valid Sequence):

What is the expected behavior?
Type annotated as Sequence should work with len(), subscription, etc.. A more subtle bug the generator causes is that data_points can only be iterated over once, which will consume the generator.

What is the actual behavior?

TypeError: object of type 'generator' has no len()

Additional context
Possible fixes

I'd recommend adding calls to list() or tuple() to convert the generator to a sequence or alter _ViewInstrumentMatch.collect() to return a sequence itself. I think this is appropriate as the generators are actually holding SDK locks which can remain locked if the generator is not consumed.

@aabmass aabmass added bug Something isn't working sdk Affects the SDK package. metrics labels Nov 4, 2022
@lancetarn
Copy link
Contributor

This seems like an approachable first issue for me, would give me some introduction to the metrics implementation and force me to get the project up and running. Do you mind if I give it a shot?

@ocelotl
Copy link
Contributor

ocelotl commented Nov 16, 2022

Dumb question, can't we just change Sequence to Iterable? (would that be a breaking change?)

I am more concerned with this statement, @aabmass:

I think this is appropriate as the generators are actually holding SDK locks which can remain locked if the generator is not consumed.

Can this produce hard-to-find bugs? Is there a way to test if this actually happens?

lancetarn added a commit to TelemetryHub/opentelemetry-python that referenced this issue Nov 22, 2022
- Fix open-telemetry#3021
- Remove metric branch reference from CONTRIBUTING.md
lancetarn added a commit to TelemetryHub/opentelemetry-python that referenced this issue Nov 22, 2022
- Fix open-telemetry#3021
- Remove metric branch reference from CONTRIBUTING.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working metrics sdk Affects the SDK package.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants