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

MAX_DIMENSIONS should be 9, not 10 #19

Closed
dlenski opened this issue Mar 25, 2020 · 2 comments · Fixed by #20
Closed

MAX_DIMENSIONS should be 9, not 10 #19

dlenski opened this issue Mar 25, 2020 · 2 comments · Fixed by #20

Comments

@dlenski
Copy link

dlenski commented Mar 25, 2020

Per the upstream docs:

A DimensionSet MUST NOT contain more than 9 dimension keys.

Currently, you're setting MAX_DIMENSIONS to 10:

… and then using that to truncate the dimension set to 10 entries when serializing, e.g.:

def serialize(context: MetricsContext) -> str:
dimension_keys = []
dimensions_properties: Dict[str, str] = {}
for dimension_set in context.get_dimensions():
keys = list(dimension_set.keys())
dimension_keys.append(keys[0:MAX_DIMENSIONS])
dimensions_properties = {**dimensions_properties, **dimension_set}

I'm not sure that “silently truncating the dimension set” to an acceptable size is the best solution, but in any case the MAX_DIMENSIONS should be reduced to 9.

@jaredcnance
Copy link
Member

Thanks for reporting this! Definitely open to proposals on how to treat this. I've considered adding a configuration parameter that would throw exceptions or to log and silently move on in these kinds of cases.

@dlenski
Copy link
Author

dlenski commented Mar 26, 2020

Based on the Zen of Python (“Explicit is better than implicit”, “Errors should never pass silently unless explicitly silenced”, and all that) I would say that throwing an exception is probably the right default behavior.

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

Successfully merging a pull request may close this issue.

2 participants