WIP - Add firebase integration #13954
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Took a first look. I think this PR is going into a good direction and I think - once we addressed the open todos - we should merge it in and build new features on top of it.
A couple of things I think are worth looking into in the future (i.e. not in this PR):
- should we patch
collection? I'm not entirely sure what the function does but it returns the ref to the document collection so maybe it's interesting? - I'd like us to collect more information about the query. Would be great if we could somehow build a query from the individual
querycomposition functions - I think it'd be worth to look into firebase auth and file storage but these could definitely come later.
|
|
||
| const config: FirebaseInstrumentationConfig = { | ||
| firestoreSpanCreationHook: (span: Span) => { | ||
| addOriginToSpan(span, 'auto.firebase.otel.firestore'); |
There was a problem hiding this comment.
We should also set the sentry.op attribute here. It should be db.query according to the develop spec but I'll check if this is the op for all CRUD operations.
| for (let i = 0, j = files.length; i < j; i++) { | ||
| moduleFirestoreCJS.files.push( | ||
| new InstrumentationNodeModuleFile( | ||
| files[i] as string, | ||
| firestoreSupportedVersions, | ||
| moduleExports => wrapMethods(moduleExports, wrap, unwrap, tracer, firestoreSpanCreationHook), | ||
| moduleExports => unwrapMethods(moduleExports, unwrap), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think we can simplify this loop to a simple for ... of loop or .foreach, right? Should help us get rid of the two indexes we don't need.
| ): void { | ||
| const firestoreApp: FirebaseApp = reference.firestore.app; | ||
| const firestoreOptions: FirebaseOptions = firestoreApp.options; | ||
|
|
There was a problem hiding this comment.
We should also also add the db.* semantic attributes that apply for firestore. Feel free to tackle this in this PR or in a follow-up. I'll leave this up to you to decide! https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/db.md#generic-database-attributes
There was a problem hiding this comment.
At least, we should set db.system because our Sentry "Queries" performance insights module needs it
There was a problem hiding this comment.
Let me expand on this a bit: Our Queries module has some specific requirements which information the db spans need to contain. Moreover, the support is scoped to specific databases, identified by db.system. At the moment, Firestore is not supported but I think we should model the spans according to the specification for MongoDB (given both are NoSQL albeit Firestore being less "powerful").
Unfortunately, the MongoDB spec isn't yet merged but there's a PR open which probably only needs to be merged: getsentry/sentry-docs#11234. I'd recommend you give this a read and we make adjustments in follow-up PRs. To completely satisfy the spec we'll need to be able to access or reconstruct the query.
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).