gcloud logging more#182
Conversation
kevmoo
commented
May 13, 2026
kevmoo
commented
- Refactor logging and environment parsing to use package:google_cloud
- cleanup logging!
Integrates `package:google_cloud` for structured logging, trace context, and standard environment variables. Details: - Update `Logger` to extend `CloudLogger` from `package:google_cloud`. - Replace custom trace context parsing and zone keys with `TraceContextData` and standard constants. - Update `fireUp` server startup to use `TraceContextData.tryParse`. - Remove custom `removeCircular` logic and related tests (offloaded to `google_cloud`). - Upgrade `google_cloud_storage` to `^0.6.0` and add `google_cloud` dependency. - Modernize logging calls throughout (`logger.warning` with `stackTrace` etc). - Remove obsolete `server_test.dart` for custom trace parsing. - Use standard constants for environment variables (e.g., `serviceEnvironmentVariable`, `projectIdEnvironmentVariableOptions`).
![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 Firebase Functions error handling and logging infrastructure, replacing custom error classes with HttpResponseException from package:google_cloud and integrating structured logging. Key changes include updating callable and event-triggered function handlers to use the new exception type, removing deprecated error logging utilities, and updating the server startup logic to use serveHandler. The review feedback highlights critical issues, including a potential runtime error due to a missing toJson method on HttpResponseException, a regression in observability where exceptions were being swallowed, a broken dependency override path in pubspec.yaml, a missing import for currentLogger, and a suggestion to utilize the FunctionsErrorCode enum for consistent error reporting.
| return createNonStreamingResponse(result); | ||
| } on HttpsError catch (e) { | ||
| } on HttpResponseException catch (e) { | ||
| final errorPayload = e.toJson(); |
There was a problem hiding this comment.
There are two issues here:
HttpResponseExceptionfrompackage:google_clouddoes not have atoJson()method, which will cause a runtimeNoSuchMethodError.- The Firebase Callable protocol requires error responses to be wrapped in a top-level
errorkey (e.g.,{"error": {"status": "...", "message": "..."}}). Simply returning the result of a hypotheticaltoJson()would likely violate this spec.
You should manually construct the error payload using the FunctionsErrorCode enum provided at the end of this file to map the HTTP status code to the appropriate string status.
| final errorPayload = e.toJson(); | |
| } on HttpResponseException catch (e) { | |
| final errorPayload = { | |
| 'error': { | |
| 'status': FunctionsErrorCode.values | |
| .firstWhere((v) => v.httpStatusCode == e.statusCode, | |
| orElse: () => FunctionsErrorCode.unknown) | |
| .value | |
| .toUpperCase() | |
| .replaceAll('-', '_'), | |
| 'message': e.message ?? 'Unknown error', | |
| } | |
| }; |
| } catch (e) { | ||
| final errorPayload = { | ||
| 'error': {'status': 'INTERNAL', 'message': 'Internal error'}, | ||
| }; |
There was a problem hiding this comment.
This catch block for unexpected errors is a regression in observability. It swallows the exception e and its stack trace without logging them, making it impossible to diagnose server-side failures. It should also include the stackTrace parameter in the catch clause and use currentLogger.error to record the failure details before returning the generic internal error response to the client.
} catch (e, stackTrace) {
currentLogger.error('Unexpected error in callable function',
error: e, stackTrace: stackTrace);
final errorPayload = {
'error': {'status': 'INTERNAL', 'message': 'Internal error'},
};| google_cloud: | ||
| path: /Users/kevmoo/github/google-cloud-dart/pkgs/google_cloud |
There was a problem hiding this comment.
The dependency_overrides section contains a local file system path (/Users/kevmoo/...). This will cause the build to fail in CI environments and for any other developer who does not have the exact same directory structure. This override should be removed before merging.
| import 'dart:async'; | ||
| import 'dart:convert'; | ||
|
|
||
| import 'package:google_cloud/http_serving.dart'; |
There was a problem hiding this comment.
The currentLogger from package:google_cloud is used in several places in this PR, but the necessary import is missing in this file. This will lead to compilation errors when trying to use currentLogger for error logging in the callable handlers.
| import 'package:google_cloud/http_serving.dart'; | |
| import 'package:google_cloud/google_cloud.dart'; | |
| import 'package:google_cloud/http_serving.dart'; |
| enum FunctionsErrorCode { | ||
| // NOTE: These are ordered so that the first error code with a given HTTP | ||
| // status code is the one that is used when mapping from HTTP status codes. | ||
| ok('ok', 'OK', 200), | ||
| invalidArgument('invalid-argument', 'Invalid argument', 400), | ||
| failedPrecondition('failed-precondition', 'Failed precondition', 400), | ||
| outOfRange('out-of-range', 'Value out of range', 400), | ||
| unauthenticated('unauthenticated', 'Unauthenticated', 401), | ||
| permissionDenied('permission-denied', 'Permission denied', 403), | ||
| notFound('not-found', 'Resource not found', 404), | ||
| alreadyExists('already-exists', 'Resource already exists', 409), | ||
| aborted('aborted', 'Operation aborted', 409), | ||
| resourceExhausted('resource-exhausted', 'Resource exhausted', 429), | ||
| cancelled('cancelled', 'Request was cancelled', 499), | ||
| internal('internal', 'Internal error', 500), | ||
| unknown('unknown', 'Unknown error occurred', 500), | ||
| dataLoss('data-loss', 'Data loss', 500), | ||
| unimplemented('unimplemented', 'Operation not implemented', 501), | ||
| unavailable('unavailable', 'Service unavailable', 503), | ||
| deadlineExceeded('deadline-exceeded', 'Deadline exceeded', 504); | ||
|
|
||
| const FunctionsErrorCode(this.value, this.message, this.httpStatusCode); | ||
|
|
||
| /// The string value used in JSON serialization. | ||
| final String value; | ||
|
|
||
| /// The default human-readable message for this error code. | ||
| final String message; | ||
|
|
||
| /// The corresponding HTTP status code. | ||
| final int httpStatusCode; | ||
|
|
||
| /// Maps an error code value string to the corresponding enum. | ||
| static FunctionsErrorCode? fromValue(String value) { | ||
| for (final code in FunctionsErrorCode.values) { | ||
| if (code.value == value) { | ||
| return code; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The FunctionsErrorCode enum is defined here but is currently unused within this file. While it might be intended for public export, it should be utilized in the error handling logic of _handleCallable to ensure consistent error reporting that adheres to the Firebase Callable specification, or removed if it is truly redundant.
1 participant
