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

Add semantic conventions for logs and errors #432

Closed
wants to merge 3 commits into from

Conversation

vmihailenco
Copy link

@vmihailenco vmihailenco commented Jan 31, 2020

This PR is based on #427 . There was some discussion about

unique hash for the particular exception and another unique hash for the exception type and line it was thrown in the code. This is important for deduplication and aggregation.

I think this is too vendor specific - some vendors don't need this hash and it is not obvious what to hash and how. Also spans may be deduplicated and aggregated too - why this is log/error specific and not span/event specific?

Also I still think it is important to support more kinds of errors besides application exceptions. For example, core dumps from IoT devices which would get sent after the device reboots.

I could add to this list symbolication (iOS), Linux core dumps, crash reports for system X etc. It is too much work to cover everything and I simply have no clue about core dumps for IoT devices.

Using single log. prefix for logs and errors

I also considered using msg. as a prefix/namespace (e.g. msg.type, msg.severity, msg.message), but we already use messaging in #418 so it may be confusing

Support for exception cause / nested exceptions

I would like to support nested exceptions, but I don't have good ideas how to do it with data types OTEL currently supports. Adding some field like log.cause with JSON payload seems to be the only option.

@vmihailenco vmihailenco changed the title Add semantic conventions for log and errors Add semantic conventions for logs and errors Jan 31, 2020
@mwear
Copy link
Member

mwear commented Feb 11, 2020

I made the same comment on #427. It's repeated here for visibility.

I went ahead and wrote some ruby code for this proposal and #427 to see how they compare.

Example for #427

begin
  buggy_method     # raises an exception when called
rescue => e
  span.add_event(name: 'error',
                 attributes: {
                   'error.type' => e.class,
                   'error.message' => e.message,
                   'error.stack' => e.backtrace
                 })
end

Example for #432

begin
  buggy_method     # raises an exception when called
rescue => e
  span.add_event(name: 'error',
                 attributes: {
                   'log.type' => e.class,
                   'log.severity' => 'error',
                   'log.message' => e.message,
                   'frame.file' => e.backtrace_locations[0].absolute_path,
                   'frame.line' => e.backtrace_locations[0].lineno,
                   'frame.func' => e.backtrace_locations[0].label,
                   'frame.stack' => stack_to_json(e)
                 })
end

def stack_to_json(e)
  JSON.dump(
    e.backtrace_locations.map do |loc|
      { 'frame.file' => loc.absolute_path, 'frame.line' => loc.lineno, 'frame.func' => loc.label }
    end
  )
end

#427 has merits for its simplicity. It does not cover logs as #432 does, although it could be abstracted for that purpose.

I like that #432 works generically for logs with an error being a specific type of log. There is more work to transform the backtrace into json. I also noticed that frame.file, frame.line and frame.func duplicate the first entry of the frame.stack in the case of an exception. They are listed as optional, so perhaps they should be omitted when also recording frame.stack.

Both of these approaches seem viable and I'd be 👍 on either. It'd be interesting to see how this plays out in other languages.

@arminru arminru added the area:error-reporting Related to error reporting label May 12, 2020
@reyang reyang added area:semantic-conventions Related to semantic conventions spec:logs Related to the specification/logs directory release:after-ga Not required before GA release, and not going to work on before GA labels Jul 10, 2020
@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Aug 12, 2020
@tigrannajaryan
Copy link
Member

#697 is now merged. Do we still need this PR?

@Oberon00 do you mind if I assign this to you since you are already assigned to the related #521

@github-actions github-actions bot removed the Stale label Aug 13, 2020
@bogdandrutu
Copy link
Member

Closing this PR since we already have exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:error-reporting Related to error reporting area:semantic-conventions Related to semantic conventions release:after-ga Not required before GA release, and not going to work on before GA spec:logs Related to the specification/logs directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants