Instantiate a RootSpanContextManager and save it on CrashlyticsService#9890
Instantiate a RootSpanContextManager and save it on CrashlyticsService#9890rebehe wants to merge 1 commit intocrashlytics-tracesfrom
Conversation
|
![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 refactors the Crashlytics service to support multi-app isolation by replacing the global rootSpanContextManager with per-instance managers. The changes include updating the CrashlyticsService constructor, registration logic, and helper functions to utilize the instance-specific context manager. A review comment identifies a type inconsistency where tracingProvider is marked as nullable in the service constructor but non-nullable in the internal interface, which could lead to runtime issues.
| public app: FirebaseApp, | ||
| public loggerProvider: LoggerProvider, | ||
| public tracingProvider: TracerProvider | null | ||
| public tracingProvider: TracerProvider | null, |
There was a problem hiding this comment.
The tracingProvider is marked as nullable (TracerProvider | null) here, but the CrashlyticsInternal interface in types.ts defines it as non-nullable. This inconsistency can lead to runtime errors when the service is cast to CrashlyticsInternal and tracingProvider is accessed without a null check, as seen in helpers.ts. If the provider is guaranteed to be present (which is the case in the registration factories), the type should be made non-nullable here as well.
| public tracingProvider: TracerProvider | null, | |
| public tracingProvider: TracerProvider, |
rebehe
force-pushed
the
trace-boundaries-react-router
branch
from
a02fe22 to
f5e0a97
Compare
This is the more Firebase idiomatic way to address the "multiple context managers" issue and ultimately replaces the global singleton approach. This works because _registerComponent is a factory function and is guaranteed to be called once. When we save the RootSpanContextManager object reference on CrashlyticsService, all module callers of RootSpanContextManager will end up with the same instance.
rebehe
force-pushed
the
context-manager-direct-instantiation
branch
from
b5032ba to
303a12a
Compare
| ExportResult, | ||
| W3CTraceContextPropagator | ||
| } from '@opentelemetry/core'; | ||
| import { TracerProvider, trace, context } from '@opentelemetry/api'; |
There was a problem hiding this comment.
please remove unused context import
| import { CrashlyticsService } from './service'; | ||
| import { CrashlyticsInternal } from './types'; | ||
| import { rootSpanContextManager } from './tracing/root-span-context-manager'; | ||
| import { RootSpanContextManager } from './tracing/root-span-context-manager'; |
| */ | ||
| export function startNewSession(crashlytics: Crashlytics): void { | ||
| // Cast to CrashlyticsInternal to access internal loggerProvider | ||
| const { loggerProvider, tracingProvider } = |
There was a problem hiding this comment.
please remove tracingProvider if unsused
2 participants
This is the more Firebase idiomatic way to address the "multiple context managers" issue and ultimately replaces the global singleton approach. This works because _registerComponent is a factory function and is guaranteed to be called once. When we save the RootSpanContextManager object reference on CrashlyticsService, all module callers of RootSpanContextManager will end up with the same instance.