Metric attributes does not accept all types of sequences

[ericstengard]

Describe your environment

Python version: 3.10.10
Opentelemetry-python version: 1.23.0

Steps to reproduce
It is defined in types.py, https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/types.py#L18, that we can supply a sequence of say strings as an attribute to a metric. This does not work with lists and sets but using a n-tuple works.

Offending line seems to be:

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py

Line 98 in d01dbc2

aggr_key = frozenset(attributes.items())

Below is a minimal example which proves that using an n-tuple works and a list does not.

from opentelemetry import metrics
from opentelemetry.sdk.metrics import MeterProvider
from opentelemetry.sdk.metrics.export import PeriodicExportingMetricReader
from opentelemetry.sdk.metrics.export import ConsoleMetricExporter

reader = PeriodicExportingMetricReader(ConsoleMetricExporter())
meterProvider = MeterProvider(metric_readers=[reader])
metrics.set_meter_provider(meterProvider)

attributes = {"tuple_attr": ("string", "another_string"), "list_attr": ["string"]}

metrics.get_meter_provider().get_meter("my_test_meter").create_counter("test_counter").add(
    1, attributes=attributes
)

Example output from running the above minimal example:

Traceback (most recent call last):
  File "/home/<REDACTED>/test.py", line 12, in <module>
    metrics.get_meter_provider().get_meter("my_test_meter").create_counter("test_counter").add(
  File "/home/<REDACTED>/lib/python3.10/site-packages/opentelemetry/sdk/metrics/_internal/instrument.py", line 159, in add
    self._measurement_consumer.consume_measurement(
  File "/home/<REDACTED>/lib/python3.10/site-packages/opentelemetry/sdk/metrics/_internal/measurement_consumer.py", line 82, in consume_measurement
    reader_storage.consume_measurement(measurement)
  File "/home/<REDACTED>/lib/python3.10/site-packages/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py", line 120, in consume_measurement
    view_instrument_match.consume_measurement(measurement)
  File "/home/<REDACTED>/lib/python3.10/site-packages/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py", line 98, in consume_measurement
    aggr_key = frozenset(attributes.items())
TypeError: unhashable type: 'list'

What is the expected behavior?
All sequences to work as attributes.

What is the actual behavior?
Not all sequences work as attributes.

Additional context

[ericstengard]

Would it be enough to change

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py

Line 98 in d01dbc2

aggr_key = frozenset(attributes.items())

to

        aggr_key = frozenset(attributes)

Edit: Nope that doesn't seem like the way forward.

[aabmass]

I think the fix here would be to convert the attribute value to an n-tuple if it is a sequence before calling frozenset(). Would you be interested in sending a PR @ericstengard? One thing to keep in mind is performance overhead since code is called for every measurement. It might make sense to benchmark the performance difference of eagerly converting to tuple vs checking if the value is a non-hashable sequence.

[aabmass]

It is defined in types.py, https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/types.py#L18, that we can supply a sequence of say strings as an attribute to a metric. This does not work with lists and sets but using a n-tuple works.

Small note, sets are not sequences (ordered). I think we could loosen this restriction (since we convert to frozenset internally) but probably a separate issue altogether.

[ericstengard]

I think the fix here would be to convert the attribute value to an n-tuple if it is a sequence before calling frozenset(). Would you be interested in sending a PR @ericstengard? One thing to keep in mind is performance overhead since code is called for every measurement. It might make sense to benchmark the performance difference of eagerly converting to tuple vs checking if the value is a non-hashable sequence.

I might be able to set some time to look at a fix. Couple of questions:

• Is it set in stone that https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/types.py#L18 should supports lists, hence this being a bug? One possible way to go would be to let the caller do the heavy lifting and converting their object to an n-tuple if needed. Not sure that I like this but might as well float it as a suggestion.
• Would running the tests with tox and diffing the benchmarks be sufficient when deciding which implementation to go for?

[aabmass]

• Is it set in stone that https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/types.py#L18 should supports lists, hence this being a bug? One possible way to go would be to let the caller do the heavy lifting and converting their object to an n-tuple if needed. Not sure that I like this but might as well float it as a suggestion.

Not completely set in stone, we could discuss in the SIG. Changing the type annotation would be annoying for users but wouldn't change the behavior. However, we do use the same Attributes type in the trace API which accepts lists already.

• Would running the tests with tox and diffing the benchmarks be sufficient when deciding which implementation to go for?

There is a benchmarking plugin we use (examples) which would work. You could also just use timeit results. In either case please include results from your machine since Github Actions doesn't give stable benchmarking results.

[ericstengard]

• Is it set in stone that https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/types.py#L18 should supports lists, hence this being a bug? One possible way to go would be to let the caller do the heavy lifting and converting their object to an n-tuple if needed. Not sure that I like this but might as well float it as a suggestion.

Not completely set in stone, we could discuss in the SIG. Changing the type annotation would be annoying for users but wouldn't change the behavior. However, we do use the same Attributes type in the trace API which accepts lists already.

Seems like the wrong way to go then, it would be the lazy solution but not the right one. I'll try to find some time and benchmark the different proposed solutions.

[ericstengard]

Sorry for the lack of response here. Looking at

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py

Line 84 in d01dbc2

def consume_measurement(self, measurement: Measurement) -> None:

it would seem that the better long term solution is to do away with frozenset.

In the meanwhile I'd venture and say that since strings are sequences the solution to convert all fields to n-tuples falls apart since we then still have to iterate and check for the string case and not convert which is just as performance hindering as iterating over all attributes and converting all but string sequences to n-tuples (Assuming same performance hit converting a string to n-tuple as converting a list to n-tuple). Would you agree that the best solution if we have to make do with the frozenset would be to convert all non string sequences to n-tuples, considering the performance overhead?

[grvmishra788]

Hi @aabmass & @ericstengard ! I noticed this issue has been open for a while and the conversation had stopped, so I decided to take a crack at it. I've opened #5303 with a fix that uses a try-first approach. Would love to get your thoughts on it! TIA.