getsentry / relay

Sentry event forwarding and ingestion service.
https://docs.sentry.io/product/relay/
Other
319 stars 92 forks source link

Multi-valued metric tags not supported #3691

Closed elramen closed 3 months ago

elramen commented 3 months ago

The metrics developer docs say "metrics are tagged by key value pairs, where each key can have more than one value." Also the RFC says "More than one value per key is permitted if represented as a list or tuple." Multi-valued tags is also implemented in the python sdk (not sure about the other sdks).

However, it seems that relay doesn't allow multi-valued tags for single emissions, only the last value of the list/tuple sent is actually used. Have raised this issue offline but opening it here so it's not forgotten.

jjbayer commented 3 months ago

@elramen thanks, good catch! Do you have a specific use case for multi-value tags? Neither Relay nor storage support it so I will remove it from the Relay docs. As for the Python SDK docs, all the examples here show dictionaries so I think we're good?

elramen commented 3 months ago

Hey @jjbayer, I don't have a specific use case for it, just opened this issue to make sure things are aligned.

I'll remove Tuple and List from the Python SDK's type hints for tag values.

szokeasaurusrex commented 3 months ago

@elramen thanks, good catch! Do you have a specific use case for multi-value tags? Neither Relay nor storage support it so I will remove it from the Relay docs. As for the Python SDK docs, all the examples here show dictionaries so I think we're good?

@jjbayer To be clear, I believe that @elramen is referring to the tag values (i.e. the values of the tags dictionary), which in the Python docs examples are all strings. The question is referring to whether the tags could have multiple values, e.g. by setting a value in the dictionary to a list. The tags would still be defined in a dictionary

Dav1dde commented 3 months ago

A single metric emission can only have one value per key and every key must be unique. Please check the docs PR if that new explanation does make sense to you.