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.
packages/node/src/integrations/tracing/firebase/otel/patches/firestore.ts
Outdated
Show resolved
Hide resolved
packages/node/src/integrations/tracing/firebase/otel/patches/firestore.ts
Show resolved
Hide resolved
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
E2E test passes locally for me. I'll get back to you on the environment variables in CI.
Had some minor comments/nits but once we get the tests working in CI, I think we're ready to merge.
dev-packages/e2e-tests/test-applications/firebase/tests/transactions.test.ts
Outdated
Show resolved
Hide resolved
dev-packages/e2e-tests/test-applications/firebase/tests/transactions.test.ts
Outdated
Show resolved
Hide resolved
| expect(transactionEvent.spans![0]).toMatchObject(spanAddDoc); | ||
| expect(transactionEvent.spans![1]).toMatchObject(spanSetDocs); | ||
| expect(transactionEvent.spans![2]).toMatchObject(spanGetDocs); | ||
| expect(transactionEvent.spans![3]).toMatchObject(spanDeleteDoc); |
There was a problem hiding this comment.
If it turns out that this assertion becomes flaky on CI I think it's perfectly reasonable for us to ensure that all the spans are in the spans array but we don't need to rely on the specific order. We'll reconstruct the span tree in the UI anyway based on the parent/child relations and timestamps.
I think this might be flaky because I'm not sure if our Sentry<>Otel span serialization logic is order-preserving.
dev-packages/e2e-tests/test-applications/firebase/docker/firebase/firestore.rules
Outdated
Show resolved
Hide resolved
# Conflicts: # dev-packages/node-integration-tests/package.json # packages/node/src/index.ts # yarn.lock
| deleteDoc, | ||
| getDocs, | ||
| setDoc, | ||
| } from '@firebase/firestore'; |
There was a problem hiding this comment.
Before we merge, let's make sure that this doesn't end up in the .d.ts in any way.
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
|
@Lms24 is this something we still may want to pick up, or should we close this? 🤔 |
|
I think we should still support firebase (+ I think the OTel instrumentation would be nice to upstream at some point) but we might need to revisit the test setup. |
Continued work on #13954 Resolves: #13678 Adds instrumentation for Firebase / Firestore queries. Updates on top of #13954: - Removed dependencies to firebase packages (inlined simplified types) - #13954 (comment) - Imported OTEL Semconv keys from the updated OTEL package - Set `SEMANTIC_ATTRIBUTE_SENTRY_OP` to `db.query` at all times --------- Co-authored-by: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com>
5 participants
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).