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

fix wrong representation of status value in zipkin exporter #3340

Merged
merged 9 commits into from
Oct 28, 2022

Conversation

NewReStarter
Copy link
Contributor

fix #3287
Set the name of span status code to upper case, fromUnset, Ok, Error to UNSET, OK, ERROR without making breaking changes. Previous version of code names can still be parsed.

@NewReStarter NewReStarter changed the title set export Status Code name to fully upper case fix exporter wrong representation of status codes Oct 17, 2022
@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #3340 (910601a) into main (94ae231) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3340   +/-   ##
=====================================
  Coverage   77.9%   77.9%           
=====================================
  Files        164     164           
  Lines      11359   11361    +2     
=====================================
+ Hits        8859    8861    +2     
  Misses      2301    2301           
  Partials     199     199           
Impacted Files Coverage Δ
exporters/zipkin/model.go 94.7% <100.0%> (+<0.1%) ⬆️

@dmathieu
Copy link
Member

I'm not sure this is the proper fix. Even though UnmarshalJSON remains the same here, the code actually changes, and the specification does require those codes to be capitalized, but not upper-case.
See https://github.com/open-telemetry/opentelemetry-specification/blob/fa36d43c4fe19207e653470d4efb13ff4361f711/specification/trace/api.md#set-status

I think the uppercasing should happen within the zipkin exporter, not for all codes.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the uppercasing should happen within the zipkin exporter, not for all codes.

Agreed

@NewReStarter NewReStarter changed the title fix exporter wrong representation of status codes fix wrong representation of status value in zipkin exporter Oct 18, 2022
@NewReStarter NewReStarter force-pushed the fix-zipkin-statuscode branch from 0878205 to 62bd8a8 Compare October 18, 2022 06:35
@NewReStarter
Copy link
Contributor Author

I think the uppercasing should happen within the zipkin exporter, not for all codes.

Agreed

Thx, I didn't notice the side effect. New version of this fix has been submitted, please help do the further review.

@NewReStarter NewReStarter requested a review from MrAlias October 18, 2022 09:09
CHANGELOG.md Outdated Show resolved Hide resolved
exporters/zipkin/model.go Outdated Show resolved Hide resolved
exporters/zipkin/model_test.go Outdated Show resolved Hide resolved
exporters/zipkin/model_test.go Outdated Show resolved Hide resolved
@NewReStarter NewReStarter requested a review from MrAlias October 21, 2022 09:28
CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit 3dd4e81 into open-telemetry:main Oct 28, 2022
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 this pull request may close these issues.

Zipkin exporter and collector do not agree on wire representation of status codes
4 participants