Add SeverityNumber to Span Events

[tigrannajaryan]

The log data model introduced the notion of SeverityNumber [1] that
can be recorded for individual log records.

I believe it is equally useful for events recorded for Spans. Adding
SeverityNumber to the Span events also makes it more uniform with
standalone log model which is aligned with our understanding that
Span events are another form of log records [2].

[1] https://github.com/open-telemetry/oteps/blob/master/text/logs/0097-log-data-model.md#field-severitynumber
[2] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/glossary.md#embedded-log

[Oberon00]

I think most events don't need a severity. Severity might be a useful general semantic convention, but I don't like a new first-class API for it. We are already having discussions about the usefulness of the status code (#306), so I'm against introducing new hard-coded fields for the span. I think we should rather try to improve the story for using semantic conventions (#547).

[tigrannajaryan]

I think most events don't need a severity.

I agree. However, I think the API should help the user do the right thing. In many cases (like exception recording) it is important to tell if the event is an error or no. Having this in the API encourages the user to think and define the fact that the exception is an error (or is not). Putting it in the semantic conventions creates a bigger distance and does not emphasize enough the importance of having the error flag recorded.

Severity also is a first class concept in virtually every logging library and I think we should treat the events as logs attached to spans, so the reasons why it is a good idea that severity is a first-class concept in the logging world also apply here.

I strongly believe we need a way to indicate that a Span event is an error or no. I think this is a nice way to achieve it and also provide a more granular way to specify how severe the error is.

[Oberon00]

However, I think the API should help the user do the right thing.

Agreed. Hence my reference to the "semantic conventions as YAML" / "Typed Span" issue #547. I don't think it is the right thing to add a severity to most events.

[Oberon00]

During the discussion about removing status on #706, it occurred to me that maybe we could replace status with a severity also for spans. Instead of having error=true we could have a severity of error or also warning for something like HTTP 404. This is just an idea I'm throwing out though, I'm not sure myself if it is a good one.

CC @iNikem

[iNikem]

@Oberon00 You opposed having severity on exception event. Why do you like severity on span as a whole? :)

[Oberon00]

I'm opposed to having it as a dedicated field, but not opposed to having it as a semantic convention. EDIT: And I was also opposed to introducing severity in the same PR as exceptions, since I wanted to keep discussion of error reporting and exception reporting separate.

[iNikem]

@tedsuo Can you please add here your opinion/proposal about adding severity to Span.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[tigrannajaryan]

@open-telemetry/specs-approvers any thoughts on this proposal?

[bogdandrutu]

@tigrannajaryan I see this as a thing that we can discuss later and we can maybe add after GA, if that is not the case let me know

[tigrannajaryan]

@tigrannajaryan I see this as a thing that we can discuss later and we can maybe add after GA, if that is not the case let me know

You are right, this is correctly labeled. If we decide to move forward with this proposal it will still be backwards compatible so no rush.

[iNikem]

If this PR is for after GA, then by #792 it should be "postponed (by closing the PR and filing a tracking issue for the next milestone)"

[carlosalberto]

"by closing the PR and filing a tracking issue for the next milestone" +1 on this.

[tigrannajaryan]

If this PR is for after GA, then by #792 it should be "postponed (by closing the PR and filing a tracking issue for the next milestone)"

Sounds good to me. So this means we don't use "release:after-ga" label for PRs, right?

[Oberon00]

Closing every PR if its classified after-GA seems extreme. We basically won't be accepting outside contribution.

[iNikem]

If we have capacity to quickly decide the faith of the PR, we can do that. But if we cannot process and merge PR before GA, what's the point of having it open for months and months?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[tigrannajaryan]

Closing the PR, not actionable now.