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

Removal of multi valued Attributes #435

Closed
carlosalberto opened this issue Feb 4, 2020 · 7 comments
Closed

Removal of multi valued Attributes #435

carlosalberto opened this issue Feb 4, 2020 · 7 comments
Milestone

Comments

@carlosalberto
Copy link
Contributor

This is related to #341

Recently in the Java SIG there have been a few comments regarding whether we need this feature. A comment from the (closed) related issue:

"simple joining on comma is cheaper to index split and join and more supportable vs json arrays. I know folks are already doing this and it requires no more coordination on backend than making a json array."

and

"A concrete non-request tagging use case for array values are things like experiment IDs. Usually these are bounded to much lower cardinality, yet need to be indexed for lookup."

Wondering how much we want to look into revisiting this feature.

@Oberon00
Copy link
Member

Oberon00 commented Feb 4, 2020

Joining with comma is not always easy, e.g. when the values already contain comma. JSON encoding is possible and can be used as exporter-side implementation for array-valued attributes.

@thisthat
Copy link
Member

thisthat commented Feb 4, 2020

A further point that is worth discussing IMHO is whether we allow/drop null or empty strings inside the multi value list/array.

@Oberon00
Copy link
Member

Oberon00 commented Feb 4, 2020

@thisthat For that discussion, see also #431.

@jmacd
Copy link
Contributor

jmacd commented Feb 4, 2020

#341

@MrAlias
Copy link
Contributor

MrAlias commented Feb 10, 2020

It seems like the comment referenced in the issue description shows a need to support multi-valued attributes by providing another use-case, not remove that support.

The other comment referenced offers an alternative encoding of the multi-values in an attribute. I think this alternate encoding discussion appears to be in contrast to the serialized JSON format described in the related issue which is at the exporter level.

It seems correct to treat the encoding of the multiple values at the exporter level and leave them in a language-native structure in the API and SDK. This would mean that the API and default SDK would still need to support multi-value attributes.

If, however, the comment on the alternate encoding was intended to change the way the API handles multi-valued attributes that seems to me it would be a different discussion than removing them entirely.

I'm not seeing how these recent comments support or motivate this issue to remove multi-valued attributes. If there are new/old topics that need to be addressed that would motivate this removal can we add them here? If not, can we close this issue?

@Oberon00
Copy link
Member

I think, we can close this issue with the decision "We're not removing them but will resolve related problems in #459".

@carlosalberto
Copy link
Contributor Author

Closing via #459

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants