Skip to content

Context manager direct instantiation#9889

Open
rebehe wants to merge 140 commits intomainfrom
context-manager-direct-instantiation
Open

Context manager direct instantiation#9889
rebehe wants to merge 140 commits intomainfrom
context-manager-direct-instantiation

Conversation

@rebehe
Copy link
Copy Markdown

@rebehe rebehe commented Apr 24, 2026

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.

andrewbrook and others added 30 commits August 19, 2025 13:55
Create new telemetry package from template
* Basic scaffolding to init telemetry package

* fixed import for node
* Add support for exporting captured errors

* Clarify flush() usage
* Add ref doc generation to `telemetry` package

* fix docs

* Rename index to register

* Run `typings:public` script to generate correct `typings` value

* Update _toc.yaml

* add api-report run to yarn build

---------

Co-authored-by: Christina Holland <chholland@google.com>
* Propagate trace context to logs

* format
* Add support for collection of NextJS server-side errors

* typo

* Remove next dependency

* format

* format

* documentation

* Add next as a devDependency for types

* documentation

---------

Co-authored-by: Christina Holland <chholland@google.com>
* Add support for logging in Edge Runtime

* format

* format
* WIP react support

* WIP react support with sub-package

* Fix package structure; capture client errors

* Implement and test FirebaseTelemetry component

* docgen

* Refactor to use React component function

* Fixed API extraction for subpath

* Format

* Fix declaration file

* Fix public typings

* Fix public typings

* Fix public typings
* docs: Fix docgen script to clean out package temp folders first (#9250)

* Version Packages (#9258)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Expose setUserProperties from internal analytics instance (#9263)

* feat(ai): add support for URL context (#9254)

* docs(ai): tag code execution with "beta" tag (public preview) (#9274)

* fix(ai): Handle empty parts when streaming (#9262)

* feat(ai): Add method to send function responses in a live session (#9272)

* Update ai model name used for e2e tests (#9282)

---------

Co-authored-by: Google Open Source Bot <firebase-oss-bot@google.com>
Co-authored-by: Christina Holland <chholland@google.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Daniel La Rocque <dlarocque@google.com>
#9290)

* Introduce TelemetryOptions allowing telemetry endpoint to be specified.

* Fix typo

* update lock

* update version of firebase/app

* Normalize getTelemetry initialization

* Clean up service init
* docs: Fix docgen script to clean out package temp folders first (#9250)

* Version Packages (#9258)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Expose setUserProperties from internal analytics instance (#9263)

* feat(ai): add support for URL context (#9254)

* docs(ai): tag code execution with "beta" tag (public preview) (#9274)

* fix(ai): Handle empty parts when streaming (#9262)

* feat(ai): Add method to send function responses in a live session (#9272)

* Update ai model name used for e2e tests (#9282)

* docs(ai): promote gemini developer api and imagen to GA (#9255)

* Version Packages (#9300) release v12.4.0

For JS SDK release v12.4.0

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix(auth): export MISSING_PASSWORD in AuthErrorCodes (#9297)

* update dependency

---------

Co-authored-by: Google Open Source Bot <firebase-oss-bot@google.com>
Co-authored-by: Christina Holland <chholland@google.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Daniel La Rocque <dlarocque@google.com>
Co-authored-by: Navya Sree Arugonda <67328292+Navya-arugonda@users.noreply.github.com>
Adds App Check integration for client-sdie logging
* WIP Angular ErrorHandler

* implement ng ErrorHandler

* generate docs

* downgrade angular deps for compat

* cleanup
MaesterChestnut and others added 9 commits March 25, 2026 14:38
Co-authored-by: Chris Chestnut <cchestnut@google.com>
* To once and for all deal with complex commit merge/revert history in the crashlytics branch, created a new feature branch for traces and rebased/copied all tracing related changes from crashlytics-tracing on top of the most recent crashlytics branch.
* Manually reverted extraneous changes (e.g. accidental changes to eslint) that happened in the legacy crashlytics-tracing that were unnecessary for the tracing feature.
* Addressed presubmit errors around formatting. There are a few more presubmit errors around module resolution that will be handled in a follow up.
* Finally, ran the playground app locally and verified tracing is working and writing to Cloud via the proxy server.
* Merge latest changes from main into crashlytics (#9790)

Co-authored-by: Chris Chestnut <cchestnut@google.com>

* Merge latest changes from main into crashlytics (#9811)

* Snapshot of crashlytics tracing feature

* To once and for all deal with complex commit merge/revert history in the crashlytics branch, created a new feature branch for traces and rebased/copied all tracing related changes from crashlytics-tracing on top of the most recent crashlytics branch.
* Manually reverted extraneous changes (e.g. accidental changes to eslint) that happened in the legacy crashlytics-tracing that were unnecessary for the tracing feature.
* Addressed presubmit errors around formatting. There are a few more presubmit errors around module resolution that will be handled in a follow up.
* Finally, ran the playground app locally and verified tracing is working and writing to Cloud via the proxy server.

---------

Co-authored-by: MaesterChestnut <40321652+MaesterChestnut@users.noreply.github.com>
Co-authored-by: Chris Chestnut <cchestnut@google.com>
Co-authored-by: Chris Chestnut <cchestnut@google.com>
* Squashed commit of the following:

commit 4a07bc7
Author: Sai Sakhamuri <ssakhamu@google.com>
Date:   Thu Apr 16 12:24:20 2026 -0400

    Crashlytics Refactor for Tracing Logic (#9848)

    * refactoring out logging variable names to generalize for future tracing code

    * refactoring fetch-transport.ts out of logging folder

    * running yarn format

    * renaming SIGNAL_ATTRIBUTE_KEYS to CRASHLYTICS_ATTRIBUTE_KEYS

    * ran yarn format again

    * moving fetch-trasnport.test.ts into src level as well

commit 6a6b7b6
Author: MaesterChestnut <40321652+MaesterChestnut@users.noreply.github.com>
Date:   Mon Apr 13 10:35:24 2026 -0400

    Crashlytics merge main (#9833)

    * Merge latest changes from main into crashlytics

    * Update crashlytics package.json with new firebase dependency versions

    ---------

    Co-authored-by: Chris Chestnut <cchestnut@google.com>

commit fe2051c
Author: Rebecca He <31138589+rebehe@users.noreply.github.com>
Date:   Mon Apr 6 12:13:12 2026 -0400

    Merge latest changes from main into crashlytics (#9811)

commit e6b5ee0
Author: MaesterChestnut <40321652+MaesterChestnut@users.noreply.github.com>
Date:   Wed Apr 1 08:45:16 2026 -0400

    Merge latest changes from main into crashlytics (#9790)

    Co-authored-by: Chris Chestnut <cchestnut@google.com>

* fixing merge conflcit resolve on helpers.ts
* adding OTLP Trace Exporter for Firebase Telemetry Server

* applied formatting

* fixing fetch transport path

* providing cleaned url for regex url matching in fetch instrumentation ignore list

* resolving comments as well as refactoring for signal variable names

* changing names for different variables and allowing proxy to be used as well as telemetry server endpoint

* running yarn format

* renaming SIGNAL_ATTRIBUTE_KEYS to CRASHLYTICS_ATTRIBUTE_KEYS

* fixing fetch transport path for tracing provider

* resolving minor issues

* running yarn format
Co-authored-by: Chris Chestnut <cchestnut@google.com>
* adding resource attributes to tracing provider

* refactoring for constant keys and hardcoded values

* adding span attribute mappings through support for modern otel conventions

* fixing some syntax issues and redundancies

* adding logic for creating common span attributes

* adding changes for configOptions refactor

* making endpointUrl const in tracing provider
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 24, 2026

🦋 Changeset detected

Latest commit: 303a12a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@firebase/ai Minor
firebase Minor
@firebase/firestore Minor
@firebase/auth Minor
@firebase/analytics-interop-types Patch
@firebase/analytics Patch
@firebase/firestore-compat Patch
@firebase/auth-compat Patch
@firebase/analytics-compat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Apr 24, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the new @firebase/crashlytics package for OpenTelemetry-based telemetry, adds TemplateChatSession and responseJsonSchema support to @firebase/ai while deprecating Imagen models, and implements streaming for @firebase/data-connect. The code review identified critical race conditions and error handling flaws in the ChatSessionBase promise chaining logic that could lead to inconsistent state or unusable sessions after failures. Additionally, feedback was provided regarding ignored request options, incomplete tool aggregation for function calling, type inconsistencies in the Crashlytics service, and a suggestion for safer error message handling in user-defined functions.

Comment on lines +136 to +145
let finalResult = {} as GenerateContentResult;
await this._sendPromise;
/**
* Temporarily store multiple turns for cases like automatic function
* calling, only writing them to official history when the entire
* sequence has completed successfully.
*/
const tempHistory: Content[] = [];

this._sendPromise = this._sendPromise.then(async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a race condition when _sendMessage is called multiple times synchronously. Because this._sendPromise is awaited before being updated, multiple calls can pass the initial await (line 136) before the first one updates the promise (line 145). This leads to concurrent execution of turns and inconsistent history state. The promise should be updated synchronously to queue the next turn correctly, and the history update should be moved inside the then block to ensure atomicity.

*/
const tempHistory: Content[] = [];

this._sendPromise = this._sendPromise.then(async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If a request fails and the promise rejects, all subsequent calls to sendMessage will be skipped because then() is not called on a rejected promise. The chain should include a .catch(() => {}) before the next .then() to ensure the session remains usable after an error.

Comment on lines +229 to +304
await this._sendPromise;
/**
* Temporarily store multiple turns for cases like automatic function
* calling, only writing them to official history when the entire
* sequence has completed successfully.
*/
const tempHistory: Content[] = [];

const callGenerateContentStream =
async (): Promise<GenerateContentStreamResult> => {
let functionCalls: FunctionCall[] | undefined;
let functionCallTurnCount = 0;
const functionCallMaxTurns =
this.requestOptions?.maxSequentialFunctionCalls ??
DEFAULT_MAX_SEQUENTIAL_FUNCTION_CALLS;
let result: GenerateContentStreamResult & {
firstValue?: GenerateContentResponse;
};

// Repeats until model returns a response with no function calls
// or until `functionCallMaxTurns` is met or exceeded.
do {
let formattedContent;

if (functionCalls) {
functionCallTurnCount++;
const functionResponseParts = await this._callFunctionsAsNeeded(
functionCalls
);
formattedContent = formatNewContent(functionResponseParts);
} else {
formattedContent = formatNewContent(request);
}

tempHistory.push(formattedContent);
const formattedRequest = this._formatRequest(
formattedContent,
tempHistory
);

result = await this._callGenerateContentStream(
formattedRequest,
singleRequestOptions
);

functionCalls = this._getCallableFunctionCalls(result.firstValue);

if (
functionCalls &&
result.firstValue &&
result.firstValue.candidates &&
result.firstValue.candidates.length > 0
) {
const responseContent = {
...result.firstValue.candidates[0].content
};
if (!responseContent.role) {
responseContent.role = 'model';
}
tempHistory.push(responseContent);
}
} while (functionCalls && functionCallTurnCount < functionCallMaxTurns);

if (functionCalls && functionCallTurnCount >= functionCallMaxTurns) {
logger.warn(
`Automatic function calling exceeded the limit of` +
` ${functionCallMaxTurns} function calls. Returning last model response.`
);
}
return { stream: result.stream, response: result.response };
};

const streamPromise = callGenerateContentStream();

// Add onto the chain.
this._sendPromise = this._sendPromise
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to _sendMessage, _sendMessageStream has a race condition where multiple synchronous calls can lead to concurrent execution of turns. The promise chain should be updated synchronously without an initial await.

let functionCalls: FunctionCall[] | undefined;
let functionCallTurnCount = 0;
const functionCallMaxTurns =
this.requestOptions?.maxSequentialFunctionCalls ??
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The maxSequentialFunctionCalls option from singleRequestOptions should be considered here. It is currently ignored in favor of this.requestOptions.

Comment on lines +356 to +358
const functionDeclarationsTool = this.params?.tools?.find(
tool => (tool as FunctionDeclarationsToolType).functionDeclarations
) as FunctionDeclarationsTool;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic only considers the first tool that contains functionDeclarations. If a model response contains function calls defined across multiple tools, automatic function calling will fail or be incomplete. Consider iterating over all tools and aggregating their function declarations.

Comment on lines +412 to +414
`Error in user-defined function "${functionDeclaration.name}": ${
(e as Error).message
}`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Accessing (e as Error).message is unsafe if the thrown value is not an Error object. Use a safer check.

Suggested change
`Error in user-defined function "${functionDeclaration.name}": ${
(e as Error).message
}`
`Error in user-defined function "${functionDeclaration.name}": ${
e instanceof Error ? e.message : String(e)
}`

constructor(
public app: FirebaseApp,
public loggerProvider: LoggerProvider,
public tracingProvider: TracerProvider | null,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The tracingProvider is marked as nullable in the constructor, but it is defined as non-nullable in the CrashlyticsInternal interface in types.ts. This inconsistency can lead to runtime errors when internal APIs expect it to be present.

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 rebehe force-pushed the context-manager-direct-instantiation branch from b5032ba to 303a12a Compare April 24, 2026 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants