Skip to content
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

refactor: e2e tests using cloud trace v1 api. #2093

Merged
merged 3 commits into from
Aug 19, 2024
Merged

refactor: e2e tests using cloud trace v1 api. #2093

merged 3 commits into from
Aug 19, 2024

Conversation

ehsannas
Copy link
Contributor

@ehsannas ehsannas commented Jul 29, 2024

Add end-to-end tests. Each end-to-end test enables tracing, performs one or more Firestore operations, and then queries the Cloud Trace backend to see if the expected trace and spans have been generated.

@ehsannas ehsannas self-assigned this Jul 29, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Jul 29, 2024
@ehsannas ehsannas marked this pull request as ready for review July 29, 2024 23:47
@ehsannas ehsannas requested review from a team as code owners July 29, 2024 23:47
"system-test:named-db:emulator:rest": "FIRESTORE_NAMED_DATABASE=test-db FIRESTORE_EMULATOR_HOST=localhost:8080 FIRESTORE_PREFER_REST=true mocha build/system-test --timeout 600000",
"system-test:emulator:grpc": "FIRESTORE_EMULATOR_HOST=localhost:8080 mocha build/system-test --timeout 600000",
"system-test:named-db:emulator:grpc": "FIRESTORE_NAMED_DATABASE=test-db FIRESTORE_EMULATOR_HOST=localhost:8080 mocha build/system-test --timeout 600000",
"system-test:rest": "FIRESTORE_PREFER_REST=true mocha build/system-test --timeout 1200000",
Copy link
Contributor

Choose a reason for hiding this comment

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

why the timeout is extended? would this slow the full cycle down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it'll slow down the start-to-finish time of the "integration tests CI". This PR adds 120 tests and the tests take a while because each test does at the very least one round trip to Firestore server, and at least 2-3 round-trips to cloud trace server (because the traces don't become available in the cloud trace server immediately, so we need to retry).

I am open to suggestions about how to reduce this cost. Maybe we should run the in-memory tests on PR CI and cloud-trace backend tests on a nightly basis.

Copy link
Contributor

Choose a reason for hiding this comment

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

if only 10 minutes are added on top to run the whole system test, I think we could live with that comparing to having some part of the code not tested in PRs.

dev/system-test/tracing.ts Outdated Show resolved Hide resolved
@milaGGL
Copy link
Contributor

milaGGL commented Aug 2, 2024

it does not allow me to add comment dt non-diff lines, so here is a question:
Screenshot 2024-08-02 at 3 05 20 PM

dev/system-test/tracing.ts Show resolved Hide resolved
dev/system-test/tracing.ts Show resolved Hide resolved
@ehsannas
Copy link
Contributor Author

ehsannas commented Aug 7, 2024

Regarding your question:

For the time being, the reason this interface is here and not in firestore.d.ts is that we don't want to make the API public yet. So there's currently no public API in the Settings interface for setting the otel options. However, Settings can accept any options. So we'll use that for now.

Once we release this feature, we will move the export interface FirestoreOpenTelemetryOptions definition out of here and put it in firestore.d.ts; and we will add a openTelemetryOptions field to the Settings interface.

Java does have FirestoreOpenTelemetryOptions and a way to set it in the FirestoreOptions.

Note that there is an inherent difference between OTEL in java and node.js. In Java the Firestore SDK accepts an instance OpenTelemetry, but in Node.js it's modularized, so we need to accept an instance of a TracerProvider.

Copy link
Contributor

@milaGGL milaGGL left a comment

Choose a reason for hiding this comment

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

LGTM

@ehsannas ehsannas merged commit 4fcefde into main Aug 19, 2024
18 checks passed
@ehsannas ehsannas deleted the ehsann/otel-4 branch August 19, 2024 18:20
@ehsannas ehsannas restored the ehsann/otel-4 branch August 19, 2024 18:20
@ehsannas ehsannas deleted the ehsann/otel-4 branch August 19, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants