-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Add firebase integration #16719
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
base: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
84c5d6a
to
ea8c176
Compare
@sentry review |
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request introduces a new Firebase integration for Sentry Node, enabling automatic instrumentation of Firebase Firestore operations. The goal is to provide out-of-the-box performance monitoring and tracing for applications using Firebase, allowing developers to quickly identify and resolve performance bottlenecks within their Firebase interactions. Click to see moreKey Technical ChangesThe key technical changes include:
Architecture DecisionsThe architectural decisions include:
Dependencies and InteractionsThis integration depends on the following:
The integration interacts with the Firebase Firestore service by intercepting calls to its API. It also interacts with the Sentry backend by sending transaction events containing the generated spans. Risk ConsiderationsThe potential risks and considerations include:
Notable Implementation DetailsNotable implementation details include:
|
packages/node/src/integrations/tracing/firebase/otel/firebaseInstrumentation.ts
Show resolved
Hide resolved
} | ||
diag.error(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.
The diag.error(error?.message)
may not provide sufficient context. Consider logging the full error object or providing more descriptive error messages with operation context.
} | |
diag.error(error?.message); | |
diag.error('Firebase Firestore span creation hook failed:', error); |
packages/node/src/integrations/tracing/firebase/otel/patches/firestore.ts
Show resolved
Hide resolved
return span; | ||
} | ||
|
||
function addAttributes<AppModelType, DbModelType extends DocumentData>( | ||
span: Span, | ||
reference: CollectionReference<AppModelType, DbModelType> | DocumentReference<AppModelType, DbModelType>, | ||
): void { | ||
const firestoreApp: FirebaseApp = reference.firestore.app; | ||
const firestoreOptions: FirebaseOptions = firestoreApp.options; | ||
const json: { settings?: FirestoreSettings } = reference.firestore.toJSON() || {}; | ||
const settings: FirestoreSettings = json.settings || {}; |
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.
Consider sanitizing sensitive data from Firebase options before adding them as span attributes. ProjectId and appId may be acceptable, but other fields might contain sensitive information.
return span; | |
} | |
function addAttributes<AppModelType, DbModelType extends DocumentData>( | |
span: Span, | |
reference: CollectionReference<AppModelType, DbModelType> | DocumentReference<AppModelType, DbModelType>, | |
): void { | |
const firestoreApp: FirebaseApp = reference.firestore.app; | |
const firestoreOptions: FirebaseOptions = firestoreApp.options; | |
const json: { settings?: FirestoreSettings } = reference.firestore.toJSON() || {}; | |
const settings: FirestoreSettings = json.settings || {}; | |
const attributes: SpanAttributes = { | |
[ATTR_DB_COLLECTION_NAME]: reference.path, | |
[ATTR_DB_NAMESPACE]: firestoreApp.name, | |
[ATTR_DB_SYSTEM_NAME]: 'firebase.firestore', | |
'firebase.firestore.type': reference.type, | |
'firebase.firestore.options.projectId': firestoreOptions.projectId, | |
// Consider if these should be included for security reasons | |
// 'firebase.firestore.options.appId': firestoreOptions.appId, | |
// 'firebase.firestore.options.messagingSenderId': firestoreOptions.messagingSenderId, | |
// 'firebase.firestore.options.storageBucket': firestoreOptions.storageBucket, | |
}; |
if (typeof filePathUpdateNotifierFirebaseTools !== 'string') { | ||
throw new Error('no CONFIG_UPDATE_NOTIFIER_FIREBASE_TOOLS environment'); | ||
} | ||
|
||
try { | ||
filePathFirebaseTools = JSON.parse(filePathFirebaseTools); |
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 JSON.parse calls should be wrapped in try-catch blocks to handle malformed environment variables gracefully.
if (typeof filePathUpdateNotifierFirebaseTools !== 'string') { | |
throw new Error('no CONFIG_UPDATE_NOTIFIER_FIREBASE_TOOLS environment'); | |
} | |
try { | |
filePathFirebaseTools = JSON.parse(filePathFirebaseTools); | |
try { | |
filePathFirebaseTools = JSON.parse(filePathFirebaseTools); | |
filePathUpdateNotifierFirebaseTools = JSON.parse(filePathUpdateNotifierFirebaseTools); | |
} catch (parseError) { | |
throw new Error(`Failed to parse Firebase configuration: ${parseError.message}`); | |
} |
function createJsonFile(filePath, json) { | ||
return new Promise((resolve, reject) => { | ||
let content = JSON.stringify(json, null, 2); | ||
|
||
// replace spaces with tabs | ||
content = content.replace(/[ ]{2}/g, '\t'); | ||
|
||
fs.mkdirSync(filePath.substring(0, filePath.lastIndexOf('/')), { recursive: true }); | ||
fs.writeFile(filePath, content, function (err) { | ||
if (err) { | ||
reject(err); | ||
} else { | ||
resolve(); | ||
} | ||
}); | ||
}); |
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 file creation operations lack proper error handling and could benefit from atomic writes to prevent partial file corruption.
function createJsonFile(filePath, json) { | |
return new Promise((resolve, reject) => { | |
let content = JSON.stringify(json, null, 2); | |
// replace spaces with tabs | |
content = content.replace(/[ ]{2}/g, '\t'); | |
fs.mkdirSync(filePath.substring(0, filePath.lastIndexOf('/')), { recursive: true }); | |
fs.writeFile(filePath, content, function (err) { | |
if (err) { | |
reject(err); | |
} else { | |
resolve(); | |
} | |
}); | |
}); | |
function createJsonFile(filePath, json) { | |
return new Promise((resolve, reject) => { | |
let content = JSON.stringify(json, null, 2); | |
// replace spaces with tabs | |
content = content.replace(/[ ]{2}/g, '\t'); | |
const tempPath = filePath + '.tmp'; | |
fs.mkdirSync(filePath.substring(0, filePath.lastIndexOf('/')), { recursive: true }); | |
fs.writeFile(tempPath, content, function (err) { | |
if (err) { | |
reject(err); | |
} else { | |
fs.rename(tempPath, filePath, (renameErr) => { | |
if (renameErr) { | |
reject(renameErr); | |
} else { | |
resolve(); | |
} | |
}); | |
} | |
}); | |
}); | |
} |
dev-packages/e2e-tests/test-applications/node-firebase/tests/transactions.test.ts
Outdated
Show resolved
Hide resolved
|
||
cleanup() { | ||
echo "Stopping services..." | ||
# Gracefully stop background processes | ||
echo "Terminating background services..." | ||
if [[ -n "$firebase_pid" ]]; then | ||
kill -SIGTERM "$firebase_pid" || echo "Failed to terminate Firebase process" | ||
wait "$firebase_pid" 2>/dev/null | ||
fi | ||
if [[ -n "$nginx_pid" ]]; then | ||
kill -SIGTERM "$nginx_pid" || echo "Failed to terminate Nginx process" | ||
wait "$nginx_pid" 2>/dev/null | ||
fi | ||
if [[ -n "$npm_pid" ]]; then | ||
kill -SIGTERM "$npm_pid" || echo "Failed to terminate NPM process" | ||
wait "$npm_pid" 2>/dev/null | ||
fi |
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 cleanup function could be more robust by adding timeouts for graceful shutdown and fallback to SIGKILL if processes don't respond.
cleanup() { | |
echo "Stopping services..." | |
# Gracefully stop background processes | |
echo "Terminating background services..." | |
if [[ -n "$firebase_pid" ]]; then | |
kill -SIGTERM "$firebase_pid" || echo "Failed to terminate Firebase process" | |
wait "$firebase_pid" 2>/dev/null | |
fi | |
if [[ -n "$nginx_pid" ]]; then | |
kill -SIGTERM "$nginx_pid" || echo "Failed to terminate Nginx process" | |
wait "$nginx_pid" 2>/dev/null | |
fi | |
if [[ -n "$npm_pid" ]]; then | |
kill -SIGTERM "$npm_pid" || echo "Failed to terminate NPM process" | |
wait "$npm_pid" 2>/dev/null | |
fi | |
cleanup() { | |
echo "Stopping services..." | |
# Gracefully stop background processes with timeout | |
echo "Terminating background services..." | |
if [[ -n "$firebase_pid" ]]; then | |
kill -SIGTERM "$firebase_pid" || echo "Failed to terminate Firebase process" | |
# Wait with timeout | |
(sleep 10; kill -SIGKILL "$firebase_pid" 2>/dev/null) & | |
wait "$firebase_pid" 2>/dev/null | |
fi |
packages/node/src/integrations/tracing/firebase/otel/patches/firestore.ts
Show resolved
Hide resolved
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.
Added some comments :)
import type { FirebaseInstrumentationConfig } from './types'; | ||
|
||
const DefaultFirebaseInstrumentationConfig: FirebaseInstrumentationConfig = {}; | ||
const firestoreSupportedVersions = ['>=3.0.0 <5']; // firebase 9+ |
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.
Does this need a comma here?
Okay no, it's alright: https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation/src/types.ts#L84-L97
packages/node/src/integrations/tracing/firebase/otel/patches/firestore.ts
Show resolved
Hide resolved
* @param tracer - Opentelemetry Tracer | ||
* @param firestoreSupportedVersions - supported version of firebase/firestore | ||
* @param wrap - reference to native instrumentation wrap function | ||
* @param unwrap - reference to native instrumentation wrap function |
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.
* @param unwrap - reference to native instrumentation wrap function | |
* @param unwrap - reference to native instrumentation unwrap function |
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.
It's a minor comment but I just saw it was marked as "resolved" but the code was not changed here. Maybe wrap
is correct anyway? :D
reference: CollectionReference<AppModelType, DbModelType>, | ||
data: WithFieldValue<AppModelType>, | ||
) => Promise<DocumentReference<AppModelType, DbModelType>> { | ||
return function addDoc(original: AddDocType<AppModelType, DbModelType>) { |
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.
Maybe it's possible to patch with a JS Proxy
here (and in the other patch functions).
And I don't really understand why this needs to be recursive 🤔 It would just be good to know. Can you explain 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.
}); | ||
} | ||
|
||
function startSpan<AppModelType, DbModelType extends DocumentData>( |
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.
As Sentry exports startSpan
as well, it's maybe good to rename this function to something else to see right away that this is not a Sentry function that is called. Maybe something like startDBSpan
?
1fd4ed0
to
9dfe783
Compare
b308230
to
57bdae8
Compare
e077257
to
f3c5ca3
Compare
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.
lgtm!
@@ -0,0 +1 @@ | |||
The structure inside OTEL is to be kept as close as possible to opentelemetry plugin. |
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'd remove this README and add this as a code comment to otel/index.ts
true, | ||
); | ||
}); | ||
} |
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.
Bug: Firebase Spans Not Properly Managed
The executeContextWithSpan
function, used for asynchronous Firebase operations, incorrectly manages span lifecycle. It uses safeExecuteInTheMiddle
, which only ends the span on synchronous errors. This prevents spans from being ended when the wrapped Promise-returning function executes without immediate synchronous errors, leading to memory leaks. Additionally, spans are ended when the Promise is created, not when the async operation completes, resulting in inaccurate timing and missed error reporting for asynchronous failures.
Continued work on #13954
Resolves: #13678
Adds instrumentation for Firebase / Firestore queries.
Updates on top of #13954:
SEMANTIC_ATTRIBUTE_SENTRY_OP
todb.query
at all times