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

Require only one of exception.{type,message} for log exceptions #182

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

endorama
Copy link
Member

Change OTLP log input to identify as exception any log that has an exception message or type attribute.

Before this change only logs with both attributes where considered as exceptions.

Closes #176

otel semconv require that at lease one field between
exception.message or exception.type to be present on an application
exception.

To ensure we respect them we default on adding exception.type only
if has a value and defaulting exception.message to a constant
string when empty and no exception.type is present.
Trigger exception creation for any log message with either message or
type. This aligns to otel semconv requirements for exceptions.
@endorama endorama requested a review from a team as a code owner November 10, 2023 16:26
Copy link
Member

@kruskall kruskall left a comment

Choose a reason for hiding this comment

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

LGTM! One minor question on the additional if

Comment on lines -67 to +84
exceptionError.Exception.Type = exceptionType
if exceptionType != "" {
exceptionError.Exception.Type = exceptionType
}
Copy link
Member

Choose a reason for hiding this comment

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

question: weren't we already doing this ? exceptionType is a string so the zero value is the empty string. Am I missing something ?

Copy link
Member Author

@endorama endorama Nov 20, 2023

Choose a reason for hiding this comment

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

Before this PR exceptionType always has a value as we check presence of both exceptionMessage and exceptionType before calling this function.

With the change in the condition (from and to or) it is now possible for exceptionType to be an empty value. As the spec says that "at least one is mandatory" and we were already forcing exceptionMessage to a const when empty, I added this guard clause to not set exceptionError.Exception.Type to an empty string value.

@endorama endorama merged commit bd8f3ec into elastic:main Nov 20, 2023
4 checks passed
@endorama endorama deleted the otlp-exceptions branch November 20, 2023 11:12
@simitt simitt mentioned this pull request Dec 19, 2023
3 tasks
@kruskall kruskall self-assigned this Jan 4, 2024
@kruskall
Copy link
Member

kruskall commented Jan 4, 2024

Tested this PR comparing both before and after. I can see the "exception" was missing from the logs if one of the two field was empty. Now, it's mapped correctly.

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.

OTLP logs should only require one of exception.{type,message} for exceptions
2 participants