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

Better compliance with status spec #246

Merged
merged 3 commits into from
Jul 13, 2021
Merged

Conversation

bryannaegele
Copy link
Contributor

Statuses are not allowed to be freeform and descriptions are only acceptable for errors. For the other statuses, descriptions are to be ignored.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status

@bryannaegele bryannaegele requested a review from a team May 12, 2021 23:14
@bryannaegele bryannaegele changed the title Better comply with status compliance Better compliance with status spec May 12, 2021
@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #246 (f4b7356) into main (9110035) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
+ Coverage   36.33%   36.37%   +0.04%     
==========================================
  Files          37       37              
  Lines        3162     3164       +2     
==========================================
+ Hits         1149     1151       +2     
  Misses       2013     2013              
Flag Coverage Δ
api 63.04% <100.00%> (+0.16%) ⬆️
elixir 16.08% <0.00%> (-0.08%) ⬇️
erlang 36.37% <100.00%> (+0.04%) ⬆️
exporter 19.58% <ø> (ø)
sdk 78.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
apps/opentelemetry_api/lib/open_telemetry.ex 20.00% <ø> (ø)
apps/opentelemetry_api/lib/open_telemetry/span.ex 18.75% <ø> (ø)
...pps/opentelemetry_api/lib/open_telemetry/tracer.ex 46.15% <ø> (ø)
apps/opentelemetry_api/src/opentelemetry.erl 74.19% <100.00%> (+0.86%) ⬆️

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 9110035...f4b7356. Read the comment docs.

@bryannaegele
Copy link
Contributor Author

One callout is the spec calls for the API to have the function be called set_status taking the arguments we're currently using to build the status record and then passing to this function. I'm reluctant to make the change at this point but wanted to point it out if anyone cares.

%% developer-facing error message
message :: unicode:unicode_binary()
message = <<"">> :: unicode:unicode_binary()
Copy link
Member

Choose a reason for hiding this comment

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

Why not unicode:chardata/0? It would make the API a little bit simpler for the Erlang users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I recall, we ran into some interop issues in some places around unicode charlists versus binary, so it settled out to using the binary everywhere as the default. I don't think it was this specific place (I believe it was around headers) but I think that was the origin of just doing the unicode binary.

Copy link
Member

Choose a reason for hiding this comment

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

My only other question before approving is related, why not allow undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we'd need to check the exporter would work ok with that since it has to export a binary for that either way, but it wasn't allowed to begin with. There was always a binary value guard on it. I was just aiming to tighten down the allowed values for the status and the description with this.

@bryannaegele bryannaegele merged commit 3eff2f2 into main Jul 13, 2021
@bryannaegele bryannaegele deleted the status-spec-compliance branch July 13, 2021 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants