-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(sdk-logs,api-logs): support exceptions in Logger API #6385
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
Changes from all commits
8bb3140
b7d40db
55fbb05
dbd9198
1f108d3
c47060a
0b22cfa
9222bf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,11 @@ import type { | |
| import * as api from '@opentelemetry/api'; | ||
| import { timeInputToHrTime, InstrumentationScope } from '@opentelemetry/core'; | ||
| import type { Resource } from '@opentelemetry/resources'; | ||
| import { | ||
| ATTR_EXCEPTION_MESSAGE, | ||
| ATTR_EXCEPTION_STACKTRACE, | ||
| ATTR_EXCEPTION_TYPE, | ||
| } from '@opentelemetry/semantic-conventions'; | ||
| import type { ReadableLogRecord } from './export/ReadableLogRecord'; | ||
| import type { LogRecordLimits } from './types'; | ||
| import { isLogAttributeValue } from './utils/validation'; | ||
|
|
@@ -102,6 +107,7 @@ export class LogRecordImpl implements ReadableLogRecord { | |
| severityText, | ||
| body, | ||
| attributes = {}, | ||
| exception, | ||
| context, | ||
| } = logRecord; | ||
|
|
||
|
|
@@ -123,6 +129,9 @@ export class LogRecordImpl implements ReadableLogRecord { | |
| this._logRecordLimits = _sharedState.logRecordLimits; | ||
| this._eventName = eventName; | ||
| this.setAttributes(attributes); | ||
| if (exception != null) { | ||
| this._setException(exception); | ||
| } | ||
| } | ||
|
|
||
| public setAttribute(key: string, value?: AnyValue) { | ||
|
|
@@ -231,6 +240,54 @@ export class LogRecordImpl implements ReadableLogRecord { | |
| return value; | ||
| } | ||
|
|
||
| private _setException(exception: unknown): void { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation here is transcribing what Span#recordException() currently does, which is fine. for another PR/issue to discussI don't think this PR needs to change. I'm throwing out some ideas, because I've been around logging libs for too long. There is a part of me that wants to do more here, e.g.:
err.causeInspired by Pino's serializers for Errors (https://github.com/pinojs/pino-std-serializers?tab=readme-ov-file#serializers) some options:
other exception propertiesFor example:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Adding.
Interesting. Maybe we can propose this to the semantic conventions since it makes sense to me. What do you think?
Maybe. I can draft a PR on top of this one and create a proposal from semcon. Your suggestions sound very reasonable to me and I guess those also make sense for other languages too.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be worth looking at the discussions that led to the I think it would be worthwhile to have a follow-up issue to consider adding
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Between today and tomorrow I will do some research and create one proposal if it doesn't exist yet. I will mention you and crosslink this PR to provide some context. |
||
| let hasMinimumAttributes = false; | ||
|
|
||
| if (typeof exception === 'string' || typeof exception === 'number') { | ||
| if (!Object.hasOwn(this.attributes, ATTR_EXCEPTION_MESSAGE)) { | ||
| this.setAttribute(ATTR_EXCEPTION_MESSAGE, String(exception)); | ||
| } | ||
| hasMinimumAttributes = true; | ||
| } else if (exception && typeof exception === 'object') { | ||
| const exceptionObj = exception as { | ||
| code?: string | number; | ||
| name?: string; | ||
| message?: string; | ||
| stack?: string; | ||
| }; | ||
|
|
||
| if (exceptionObj.code) { | ||
| if (!Object.hasOwn(this.attributes, ATTR_EXCEPTION_TYPE)) { | ||
| this.setAttribute(ATTR_EXCEPTION_TYPE, exceptionObj.code.toString()); | ||
| } | ||
| hasMinimumAttributes = true; | ||
| } else if (exceptionObj.name) { | ||
| if (!Object.hasOwn(this.attributes, ATTR_EXCEPTION_TYPE)) { | ||
| this.setAttribute(ATTR_EXCEPTION_TYPE, exceptionObj.name); | ||
| } | ||
| hasMinimumAttributes = true; | ||
| } | ||
|
|
||
| if (exceptionObj.message) { | ||
| if (!Object.hasOwn(this.attributes, ATTR_EXCEPTION_MESSAGE)) { | ||
| this.setAttribute(ATTR_EXCEPTION_MESSAGE, exceptionObj.message); | ||
| } | ||
| hasMinimumAttributes = true; | ||
| } | ||
|
|
||
| if (exceptionObj.stack) { | ||
| if (!Object.hasOwn(this.attributes, ATTR_EXCEPTION_STACKTRACE)) { | ||
| this.setAttribute(ATTR_EXCEPTION_STACKTRACE, exceptionObj.stack); | ||
|
iblancasa marked this conversation as resolved.
|
||
| } | ||
| hasMinimumAttributes = true; | ||
| } | ||
| } | ||
|
|
||
| if (!hasMinimumAttributes) { | ||
| api.diag.warn(`Failed to record an exception ${exception}`); | ||
| } | ||
| } | ||
|
|
||
| private _truncateToLimitUtil(value: string, limit: number): string { | ||
| if (value.length <= limit) { | ||
| return value; | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[I'm not sure if this was discussed already.]
Whether to use "exception" or "error"? In JavaScript-land my impression is that "error" is the term for the thing we are talking about here. In Node.js, "exception" tends to lead one to Node's "uncaughtException" event. However, both terms are fine. MDN's https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error somewhat interchangeably uses both.
Given that (a) OTel specs use "exception", (b) adding this API is part of deprecating
Span#recordException, and (c) there is some overloading / possible confusion with the "error" log/severity level, I'm fine with exception. I'd be fine with the property being called "error" as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd gravitate towards
error, but the confusion with the severity level is a good point.Let's keep
exception. 👍