-
Notifications
You must be signed in to change notification settings - Fork 651
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
Improve attributes validation #460
Improve attributes validation #460
Conversation
4fca8c9 ("Add runtime validation in setAttribute (open-telemetry#348)") added a robust attribute validation using numbers.Number to validate numeric types. Although the approach is correct, it presents some complications because Complex, Fraction and Decimal are accepted because they are Numbers. This presents a problem to the exporters because they will have to consider all these different cases when converting attributes to the underlying exporter representation. This commit simplifies the logic by accepting only int and float as numeric values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return "invalid type" | ||
|
||
# int and float are both numeric types, allow mixed arrays of those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and Sequence[Union[int, float]]
above: do we really want to allow mixed-type array attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. #348 added a specific test for that. The specification is not that clear, it talks about numeric
as a primitive type [1]. Having a second thought, float and int are treated different in the exporters, so I think it would make sense not to avoid mixed arrays of those. If you agree I could update this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want this to block the PR, up to you whether to update this in this PR or another one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR to avoid that kind of mixed attributes.
Nice, thanks! |
Total Changelog: Documentations has been significantly overhauled, including: * a getting started guide * examples has been consolidated to an docs/examples folder * several minor improvements to the examples in each extension's readme. - Adding Correlation Context API and propagator ([open-telemetry#471](open-telemetry#471)) - Adding a global configuration module to simplify setting and getting globals ([open-telemetry#466](open-telemetry#466)) - Rename metric handle to bound metric ([open-telemetry#470](open-telemetry#470)) - Moving resources to sdk ([open-telemetry#464](open-telemetry#464)) - Implementing propagators to API to use context ([open-telemetry#446](open-telemetry#446)) - Implement observer instrument for metrics ([open-telemetry#425](open-telemetry#425)) - Adding named meters, removing batchers ([open-telemetry#431](open-telemetry#431)) - Renaming TraceOptions to TraceFlags ([open-telemetry#450](open-telemetry#450)) - Renaming TracerSource to TraceProvider ([open-telemetry#441](open-telemetry#441)) - Adding Correlation Context SDK and propagator ([open-telemetry#471](open-telemetry#471)) - Adding OT Collector metrics exporter ([open-telemetry#454](open-telemetry#454)) - Improve validation of attributes ([open-telemetry#460](open-telemetry#460)) - Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span() (open-telemetry#469) ([open-telemetry#469](open-telemetry#469)) - Adding named meters, removing batchers ([open-telemetry#431](open-telemetry#431)) - Make counter and MinMaxSumCount aggregators thread safe ([open-telemetry#439](open-telemetry#439)) - Initial release. Support is included for both trace and metrics.
Total Changelog: Documentations has been significantly overhauled, including: * a getting started guide * examples has been consolidated to an docs/examples folder * several minor improvements to the examples in each extension's readme. - Adding Correlation Context API and propagator ([open-telemetry#471](open-telemetry#471)) - Adding a global configuration module to simplify setting and getting globals ([open-telemetry#466](open-telemetry#466)) - Rename metric handle to bound metric ([open-telemetry#470](open-telemetry#470)) - Moving resources to sdk ([open-telemetry#464](open-telemetry#464)) - Implementing propagators to API to use context ([open-telemetry#446](open-telemetry#446)) - Implement observer instrument for metrics ([open-telemetry#425](open-telemetry#425)) - Adding named meters, removing batchers ([open-telemetry#431](open-telemetry#431)) - Renaming TraceOptions to TraceFlags ([open-telemetry#450](open-telemetry#450)) - Renaming TracerSource to TraceProvider ([open-telemetry#441](open-telemetry#441)) - Adding Correlation Context SDK and propagator ([open-telemetry#471](open-telemetry#471)) - Adding OT Collector metrics exporter ([open-telemetry#454](open-telemetry#454)) - Improve validation of attributes ([open-telemetry#460](open-telemetry#460)) - Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span() (open-telemetry#469) ([open-telemetry#469](open-telemetry#469)) - Adding named meters, removing batchers ([open-telemetry#431](open-telemetry#431)) - Make counter and MinMaxSumCount aggregators thread safe ([open-telemetry#439](open-telemetry#439)) - Initial release. Support is included for both trace and metrics.
* chore: move members list out of community repo * chore: minor
#348 added some runtime checks for attributes values, however I think this could create some issues to the exporter because it's accepting types as fractions, decimal and complex. This PR changes the logic to only accept int and float, otherwise exporter will have to handle all those different datatypes, something that is clearly not done now:
opentelemetry-python/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py
Lines 266 to 273 in 005575e
opentelemetry-python/ext/opentelemetry-ext-zipkin/src/opentelemetry/ext/zipkin/__init__.py
Lines 157 to 163 in 005575e
opentelemetry-python/ext/opentelemetry-ext-otcollector/src/opentelemetry/ext/otcollector/util.py
Lines 65 to 74 in 005575e
I'm aware of open-telemetry/opentelemetry-specification#425, the outcome appears to be that
numeric
refers toint
andfloat
types.This PR also handles the case of an empty key open-telemetry/opentelemetry-specification#459.
There are still some things this PR is not handling: