Improve attributes validation by mauriciovasquezbernal · Pull Request #460 · open-telemetry/opentelemetry-python

mauriciovasquezbernal · 2020-03-04T14:14:55Z

#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

    
           if isinstance(attr, bool): 
        
               return jaeger.Tag(key=key, vBool=attr, vType=jaeger.TagType.BOOL) 
        
           if isinstance(attr, str): 
        
               return jaeger.Tag(key=key, vStr=attr, vType=jaeger.TagType.STRING) 
        
           if isinstance(attr, int): 
        
               return jaeger.Tag(key=key, vLong=attr, vType=jaeger.TagType.LONG) 
        
           if isinstance(attr, float): 
        
               return jaeger.Tag(key=key, vDouble=attr, vType=jaeger.TagType.DOUBLE)

opentelemetry-python/ext/opentelemetry-ext-zipkin/src/opentelemetry/ext/zipkin/__init__.py

Lines 157 to 163 in 005575e

    
           if isinstance(attribute_value, (int, bool, float)): 
        
               value = str(attribute_value) 
        
           elif isinstance(attribute_value, str): 
        
               value = attribute_value[:128] 
        
           else: 
        
               logger.warning("Could not serialize tag %s", attribute_key) 
        
               continue

opentelemetry-python/ext/opentelemetry-ext-otcollector/src/opentelemetry/ext/otcollector/util.py

Lines 65 to 74 in 005575e

    
           if isinstance(value, bool): 
        
               pb_attributes.attribute_map[key].bool_value = value 
        
           elif isinstance(value, int): 
        
               pb_attributes.attribute_map[key].int_value = value 
        
           elif isinstance(value, str): 
        
               pb_attributes.attribute_map[key].string_value.value = value 
        
           elif isinstance(value, float): 
        
               pb_attributes.attribute_map[key].double_value = value 
        
           else: 
        
               pb_attributes.attribute_map[key].string_value.value = str(value)

I'm aware of open-telemetry/opentelemetry-specification#425, the outcome appears to be that numeric refers to int and float 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:

Validation for attributes in links and events.
Accept None as value to delete an attribute.

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.

codecov-io · 2020-03-04T14:28:35Z

Codecov Report

Merging #460 into master will increase coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
+ Coverage   88.65%   88.98%   +0.33%     
==========================================
  Files          42       43       +1     
  Lines        2141     2216      +75     
  Branches      245      248       +3     
==========================================
+ Hits         1898     1972      +74     
- Misses        170      173       +3     
+ Partials       73       71       -2

Impacted Files	Coverage Δ
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`92.51% <100%> (+0.51%)`	⬆️
.../src/opentelemetry/sdk/metrics/export/aggregate.py	`100% <0%> (ø)`	⬆️
...app/src/opentelemetry_example_app/flask_example.py	`100% <0%> (ø)`	⬆️
...pentelemetry/context/propagation/httptextformat.py
.../context/propagation/tracecontexthttptextformat.py
.../opentelemetry/context/propagation/binaryformat.py
.../src/opentelemetry/context/propagation/__init__.py
...pi/src/opentelemetry/trace/propagation/__init__.py	`84.61% <0%> (ø)`
...etry-api/src/opentelemetry/propagators/__init__.py	`75% <0%> (ø)`
.../opentelemetry/trace/propagation/httptextformat.py	`100% <0%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 888bed9...4597838. Read the comment docs.

toumorokoshi

Makes sense, thanks!

c24t

LGTM

c24t · 2020-03-09T19:18:27Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+        if first_element_type not in (bool, str, int, float):
            return "invalid type"

+        # int and float are both numeric types, allow mixed arrays of those


This and Sequence[Union[int, float]] above: do we really want to allow mixed-type array attributes?

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.

[1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#set-attributes.

I don't want this to block the PR, up to you whether to update this in this PR or another one.

I updated the PR to avoid that kind of mixed attributes.

toumorokoshi · 2020-03-10T05:14:43Z

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.

* chore: move members list out of community repo * chore: minor

mauriciovasquezbernal added 2 commits March 4, 2020 08:32

sdk: Check for empty or null attibute key

b0f858a

mauriciovasquezbernal requested a review from a team March 4, 2020 14:14

toumorokoshi approved these changes Mar 5, 2020

View reviewed changes

mauriciovasquezbernal added sdk Affects the SDK package. needs reviewers PRs with this label are ready for review and needs people to review to move forward. labels Mar 9, 2020

c24t approved these changes Mar 9, 2020

View reviewed changes

avoid mixed int/float sequence attributes

4597838

toumorokoshi merged commit 9850fb3 into open-telemetry:master Mar 10, 2020

toumorokoshi mentioned this pull request Mar 16, 2020

Changelog and version updates for 0.5b0 release #498

Merged

mauriciovasquezbernal deleted the mauricio/improve-attributes-validation branch April 14, 2020 21:50

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

chore: move members list out of community repo (open-telemetry#460)

91fd682

* chore: move members list out of community repo * chore: minor

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve attributes validation#460

Improve attributes validation#460
toumorokoshi merged 3 commits intoopen-telemetry:masterfrom
kinvolk:mauricio/improve-attributes-validation

mauriciovasquezbernal commented Mar 4, 2020

Uh oh!

codecov-io commented Mar 4, 2020 •

edited

Loading

Uh oh!

toumorokoshi left a comment

Uh oh!

c24t left a comment

Uh oh!

c24t Mar 9, 2020

Uh oh!

mauriciovasquezbernal Mar 9, 2020

Uh oh!

c24t Mar 9, 2020

Uh oh!

mauriciovasquezbernal Mar 10, 2020

Uh oh!

toumorokoshi commented Mar 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if isinstance(attr, bool):
	return jaeger.Tag(key=key, vBool=attr, vType=jaeger.TagType.BOOL)
	if isinstance(attr, str):
	return jaeger.Tag(key=key, vStr=attr, vType=jaeger.TagType.STRING)
	if isinstance(attr, int):
	return jaeger.Tag(key=key, vLong=attr, vType=jaeger.TagType.LONG)
	if isinstance(attr, float):
	return jaeger.Tag(key=key, vDouble=attr, vType=jaeger.TagType.DOUBLE)

	if isinstance(attribute_value, (int, bool, float)):
	value = str(attribute_value)
	elif isinstance(attribute_value, str):
	value = attribute_value[:128]
	else:
	logger.warning("Could not serialize tag %s", attribute_key)
	continue

	if isinstance(value, bool):
	pb_attributes.attribute_map[key].bool_value = value
	elif isinstance(value, int):
	pb_attributes.attribute_map[key].int_value = value
	elif isinstance(value, str):
	pb_attributes.attribute_map[key].string_value.value = value
	elif isinstance(value, float):
	pb_attributes.attribute_map[key].double_value = value
	else:
	pb_attributes.attribute_map[key].string_value.value = str(value)

Conversation

mauriciovasquezbernal commented Mar 4, 2020

Uh oh!

codecov-io commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

c24t Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

mauriciovasquezbernal Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

c24t Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

mauriciovasquezbernal Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

toumorokoshi commented Mar 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Mar 4, 2020 •

edited

Loading