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

feat(common): Add catch-all status code variant for SpanStatus #1737

Closed
wants to merge 1 commit into from

Conversation

TBS1996
Copy link
Contributor

@TBS1996 TBS1996 commented Jan 11, 2023

#skip-changelog

Adds a status to SpanStatus that can hold an unknown status code. Useful for forward compatibility.

#1639

@TBS1996 TBS1996 changed the title add other field to spanstatus feat(common): Add catch-all status code variant for SpanStatus Jan 11, 2023
@jjbayer
Copy link
Member

jjbayer commented Jan 17, 2023

What would have to happen here is that for external relays, we pass any string on to the next relay, but for processing relays, we set an error on the event. But it's tricky to get access to the processing_enabled flag from deserialization.

Because we already have error handling in FromValue, the worst thing that can happen if we keep things as-is is that transactions are reported with the wrong span status. That is, the event is not actually dropped, and the error is even reported through meta (see linked code). I think that in such a case we can simply ask the user hosting the external relay to upgrade.

TL;DR: I suggest we close this PR and leave the behavior as-is.

Err(_) => {
meta.add_error(Error::expected("a trace status"));
meta.set_original_value(Some(value));
Annotated(None, meta)
}

@TBS1996 TBS1996 closed this Jan 19, 2023
@jan-auer jan-auer deleted the feat/spanstatus branch February 1, 2023 10:02
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.

2 participants