feat(crashlytics): refactor to support OpenTelemetry SDK integration, log batching, and custom options - #10232
feat(crashlytics): refactor to support OpenTelemetry SDK integration, log batching, and custom options#10232bryanatkinson wants to merge 10 commits into
Conversation
Adds the `instrumentation` flag to CrashlyticsOptions to allow web developers to easily enable full OpenTelemetry browser auto-instrumentation and global logger provider registration. TAG=agy CONV=82994c94-bc1d-4502-9a3d-b6fe1d4d4970
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry Power User APIs and a lightweight MicroOtelLoggerProvider to Firebase Crashlytics, allowing for custom loggers, global providers, and extra processors. Feedback on these changes focuses on enhancing the robustness of the lightweight provider, including initializing undefined log attributes, supporting nested objects via kvListValue, correctly parsing Date and HrTime timestamps, and checking response.ok during fetch retries. Additionally, it is recommended to align the exposed LoggerProvider types with the @opentelemetry/api-logs package to support custom API providers cleanly.
| if (logRecord.attributes) { | ||
| for (const [key, value] of Object.entries(dynamicAttributes)) { | ||
| if (value !== undefined && logRecord.attributes[key] === undefined) { | ||
| logRecord.attributes[key] = value as AttributeValue; | ||
| } | ||
| } | ||
|
|
||
| const cachedIid = this.attributesStore.getCachedInstallationId(); | ||
| if ( | ||
| cachedIid && | ||
| logRecord.attributes[ATTR_KEY_INSTALLATION_ID] === undefined | ||
| ) { | ||
| logRecord.attributes[ATTR_KEY_INSTALLATION_ID] = cachedIid; | ||
| } | ||
| } |
There was a problem hiding this comment.
If logRecord.attributes is undefined, the processor currently skips adding any dynamic Firebase attributes (such as session ID, app version, and installation ID). We should initialize logRecord.attributes to an empty object if it is undefined to ensure that all logs are correctly enriched with Firebase metadata.
if (!logRecord.attributes) {
logRecord.attributes = {};
}
for (const [key, value] of Object.entries(dynamicAttributes)) {
if (value !== undefined && logRecord.attributes[key] === undefined) {
logRecord.attributes[key] = value as AttributeValue;
}
}
const cachedIid = this.attributesStore.getCachedInstallationId();
if (
cachedIid &&
logRecord.attributes[ATTR_KEY_INSTALLATION_ID] === undefined
) {
logRecord.attributes[ATTR_KEY_INSTALLATION_ID] = cachedIid;
}| export interface OtlpAttributeValue { | ||
| stringValue?: string; | ||
| boolValue?: boolean; | ||
| intValue?: number; | ||
| doubleValue?: number; | ||
| arrayValue?: { values: OtlpAttributeValue[] }; | ||
| } |
There was a problem hiding this comment.
Add kvListValue to the OtlpAttributeValue interface to support nested maps/objects in custom attributes.
| export interface OtlpAttributeValue { | |
| stringValue?: string; | |
| boolValue?: boolean; | |
| intValue?: number; | |
| doubleValue?: number; | |
| arrayValue?: { values: OtlpAttributeValue[] }; | |
| } | |
| export interface OtlpAttributeValue { | |
| stringValue?: string; | |
| boolValue?: boolean; | |
| intValue?: number; | |
| doubleValue?: number; | |
| arrayValue?: { values: OtlpAttributeValue[] }; | |
| kvListValue?: { values: OtlpKeyValue[] }; | |
| } |
| function formatOtlpAttributeValue(val: unknown): OtlpAttributeValue { | ||
| if (typeof val === 'string') { | ||
| return { stringValue: val }; | ||
| } | ||
| if (typeof val === 'boolean') { | ||
| return { boolValue: val }; | ||
| } | ||
| if (typeof val === 'number') { | ||
| return Number.isInteger(val) ? { intValue: val } : { doubleValue: val }; | ||
| } | ||
| if (Array.isArray(val)) { | ||
| return { | ||
| arrayValue: { | ||
| values: val.map(v => formatOtlpAttributeValue(v)) | ||
| } | ||
| }; | ||
| } | ||
| return { stringValue: String(val) }; | ||
| } |
There was a problem hiding this comment.
In OpenTelemetry, custom attributes can be nested maps/objects. Currently, formatOtlpAttributeValue falls back to String(val) for objects, which serializes them as "[object Object]" and causes data loss. We should support kvListValue to correctly serialize nested objects.
function formatOtlpAttributeValue(val: unknown): OtlpAttributeValue {
if (typeof val === 'string') {
return { stringValue: val };
}
if (typeof val === 'boolean') {
return { boolValue: val };
}
if (typeof val === 'number') {
return Number.isInteger(val) ? { intValue: val } : { doubleValue: val };
}
if (Array.isArray(val)) {
return {
arrayValue: {
values: val.map(v => formatOtlpAttributeValue(v))
}
};
}
if (typeof val === 'object' && val !== null) {
return {
kvListValue: {
values: formatOtlpAttributes(val as Record<string, unknown>)
}
};
}
return { stringValue: String(val) };
}| const timeNano = logRecord.timestamp | ||
| ? String( | ||
| typeof logRecord.timestamp === 'number' | ||
| ? logRecord.timestamp * 1000000 | ||
| : Date.now() * 1000000 | ||
| ) | ||
| : String(Date.now() * 1000000); |
There was a problem hiding this comment.
The logRecord.timestamp can be a Date object or an HrTime array (e.g., from auto-instrumentation libraries). Currently, the check typeof logRecord.timestamp === 'number' fails for these types, causing the logger to fall back to Date.now(), which discards the actual timestamp of the log. We should handle all TimeInput types correctly.
let timeNano: string;
if (logRecord.timestamp instanceof Date) {
timeNano = String(logRecord.timestamp.getTime() * 1000000);
} else if (typeof logRecord.timestamp === 'number') {
timeNano = String(logRecord.timestamp * 1000000);
} else if (Array.isArray(logRecord.timestamp)) {
timeNano = String(logRecord.timestamp[0] * 1000000000 + logRecord.timestamp[1]);
} else {
timeNano = String(Date.now() * 1000000);
}| const sendRequest = async (retriesLeft = 1): Promise<void> => { | ||
| try { | ||
| await fetch(this.endpointUrl, { | ||
| method: 'POST', | ||
| headers, | ||
| body: payloadStr | ||
| }); | ||
| } catch (err) { | ||
| if (retriesLeft > 0) { | ||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||
| return sendRequest(retriesLeft - 1); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
The fetch API does not throw an error on non-ok HTTP status codes (like 5xx or 429). To ensure robust log delivery, we should check response.ok and retry on transient failures.
| const sendRequest = async (retriesLeft = 1): Promise<void> => { | |
| try { | |
| await fetch(this.endpointUrl, { | |
| method: 'POST', | |
| headers, | |
| body: payloadStr | |
| }); | |
| } catch (err) { | |
| if (retriesLeft > 0) { | |
| await new Promise(resolve => setTimeout(resolve, 1000)); | |
| return sendRequest(retriesLeft - 1); | |
| } | |
| } | |
| }; | |
| const sendRequest = async (retriesLeft = 1): Promise<void> => { | |
| try { | |
| const response = await fetch(this.endpointUrl, { | |
| method: 'POST', | |
| headers, | |
| body: payloadStr | |
| }); | |
| if (!response.ok && retriesLeft > 0) { | |
| await new Promise(resolve => setTimeout(resolve, 1000)); | |
| return sendRequest(retriesLeft - 1); | |
| } | |
| } catch (err) { | |
| if (retriesLeft > 0) { | |
| await new Promise(resolve => setTimeout(resolve, 1000)); | |
| return sendRequest(retriesLeft - 1); | |
| } | |
| } | |
| }; |
| import { AnyValueMap, SeverityNumber, Logger } from '@opentelemetry/api-logs'; | ||
| import { LoggerProvider } from '@opentelemetry/sdk-logs'; |
There was a problem hiding this comment.
Since Crashlytics now supports custom API LoggerProviders (such as MicroOtelLoggerProvider), getOtelLoggerProvider should return the API LoggerProvider from @opentelemetry/api-logs instead of the SDK LoggerProvider from @opentelemetry/sdk-logs. This avoids type lies and potential runtime errors for consumers.
| import { AnyValueMap, SeverityNumber, Logger } from '@opentelemetry/api-logs'; | |
| import { LoggerProvider } from '@opentelemetry/sdk-logs'; | |
| import { AnyValueMap, SeverityNumber, Logger, LoggerProvider } from '@opentelemetry/api-logs'; |
| import { LoggerProvider } from '@opentelemetry/sdk-logs'; | ||
| import { Logger } from '@opentelemetry/api-logs'; |
There was a problem hiding this comment.
Align the loggerProvider type in CrashlyticsInternal with @opentelemetry/api-logs's LoggerProvider to support lightweight or custom API providers.
| import { LoggerProvider } from '@opentelemetry/sdk-logs'; | |
| import { Logger } from '@opentelemetry/api-logs'; | |
| import { LoggerProvider, Logger } from '@opentelemetry/api-logs'; |
…on, kvListValue objects, timestamp parsing, and fetch retry status check
…ith --bundleConfigAsCjs
1 participant
Discussion
Description
Refactors
@firebase/crashlyticsto support direct OpenTelemetry (OTel) SDK integration, enhanced log batching/flushing capabilities, and additional developer configuration options.Key changes:
resolveLoggerProviderallowing zero-config lightweight logging viaMicroOtelLoggerProviderwhile supporting full OTel SDKLoggerProviderwith custom processors, exporters, and resources.loggerProvider,logger,extraProcessors,extraExporters,useGlobalLoggerProvider,registerGlobalLoggerProvider, andresourceinCrashlyticsOptions. ExposedgetOtelLoggerProviderandgetOtelLoggerhelper functions.FirebaseAttributesProcessorand streamlined log batching/flushing.CrashlyticsOptions.Testing
test:node) and Browser (test:browservia Karma).src/api.test.tscovering OpenTelemetry power user APIs, custom logger providers, custom loggers, and extra processors.API Changes
Added the following non-breaking public API additions to
@firebase/crashlytics:CrashlyticsOptionswith optional fields:loggerProvider,logger,extraProcessors,extraExporters,useGlobalLoggerProvider,registerGlobalLoggerProvider,resource, andinstrumentation.getOtelLoggerProvider(crashlytics: Crashlytics)andgetOtelLogger(crashlytics: Crashlytics).