-
Notifications
You must be signed in to change notification settings - Fork 19
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
OTLP telemetry exporter #213
Conversation
if (parentCtx) { | ||
this.request = parentCtx.request; | ||
this.authenticatedUser = parentCtx.authenticatedUser; | ||
this.authenticatedRoles = parentCtx.authenticatedRoles; | ||
this.assumedRole = parentCtx.assumedRole; | ||
this.workflowUUID = parentCtx.workflowUUID; | ||
} | ||
this.logger = new DBOSLogger(logger, this); |
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.
This fix a bug where the metadata would be missing if provided from the parentCtx
info(logEntry: any, metadata?: ContextualMetadata): void { | ||
if (typeof logEntry === "string") { | ||
this.logger.info(logEntry, metadata); | ||
} else { |
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.
Is this what Winston does by default when it receives non-string input? What were we doing before?
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.
Here's the set of inputs Winston accepts for its log methods:
interface LeveledLogMethod {
(message: string, callback: LogCallback): Logger;
(message: string, meta: any, callback: LogCallback): Logger;
(message: string, ...meta: any[]): Logger;
(message: any): Logger;
(infoObject: object): Logger;
}
We have always been using either (message: string, ...meta: any[]): Logger;
or (message: any): Logger;
@@ -74,7 +74,8 @@ export async function login(username: string): Promise<number> { | |||
const response = await axios.request(deviceCodeRequest); | |||
deviceCodeResponse = response.data as DeviceCodeResponse; | |||
} catch (e) { | |||
logger.error(new Error(`failed to log in: ${(e as Error).message}`)); | |||
(e as Error).message = `failed to log in: ${(e as Error).message}`; |
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.
What does this do in practice? Does it print the original stack trace?
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.
Yes we just improves the message
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.
Compared to the original code (not the first PR), it seems it improves the message vs allowing the logger to make the same improvement?
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.
What would allow the logger to make the same improvement
mean here? (The idea is to keep the logger interface simple and as close as Winston as possible, i.e., for errors, you can pass an error object, a string message or a JSON.stringify
-able object)
export(signal: TelemetrySignal[]): Promise<void>; | ||
} | ||
|
||
export class TelemetryExporter implements ITelemetryExporter { |
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.
Do we have to change the configuration file at all to support this new one-exporter, two-endpoint structure?
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.
Nope, the current way of loading the configuration supports this change transparently.
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.
Sorry if I wasn't clear: the update to the configuration types is done here
if (typeof logEntry === "string") { | ||
this.logger.debug(logEntry, metadata); | ||
} else { | ||
this.logger.debug(JSON.stringify(logEntry), metadata); |
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 suppose we wouldn't catch errors thrown from stringify (for circular references, etc.)?
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.
The contract would be that we ask users to give use objects that can be JSON.stringify
-ed. Under the hood, Winston seems to be more flexible, but it is also really complicated..
This PR updates the telemetry subsystem to support 1) formatting log entries to OTLP and 2) export logs/traces to configurable OTLP backends.
For instance, with a service that accepts both traces and logs (like the OTel collector), the DBOS SDK can be configure to export both signals to this service:
Note this setup works out of the box with Jaeger and the OTel collector (see screenshots bellow.)
Also note
opentelemetry-js
logs API is experimental, so I fixed the version we use to0.41.2
Details
LogRecord
and push it to our collectorLogRecord
andSpan
Others
Thoughts
Tests
☑️ Unit tests: ideally we would be able to intercept outgoing HTTP requests and expect the payload to conform our expectations. This seems to be quite some work so pushing that for later.
✅ Tested with DBOS cloud integration tests.
✅ Logs w/o metadata:
✅ Logs w/ metadata:
✅ Logs & traces exported to a local OTel collector
✅ Traces exported to a local Jaeger: