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

fix: Create Span hierarchy using parent Span #1580

Merged
merged 7 commits into from
Sep 19, 2024

Conversation

jimit-j-shah
Copy link
Contributor

@jimit-j-shah jimit-j-shah commented Sep 18, 2024

  • Replaced usage of SpanContext w/ Span when expressing span nesting relationship
  • Bifurcated ReadWriteTransactionCallable impl to be trace-aware to execute correctly when tracing is disabled.
  • Limited the direct reliance on OTel API instead of TraceUtil since there's no automatic context propagation possible for the Executor used by RetryHelper provided by gax. So we share the parentSpan to manually instrument the hierarchy of spans as described in the official docs.

@jimit-j-shah jimit-j-shah requested review from a team as code owners September 18, 2024 17:35
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the googleapis/java-datastore API. labels Sep 18, 2024
@jimit-j-shah jimit-j-shah changed the title fix: Replace use of TraceUtil.SpanContext w/ TraceUtil.Context fix: Create Span hierarchy using parent Span Sep 18, 2024
Copy link

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

LGTM

@jimit-j-shah jimit-j-shah merged commit e4b4af0 into tracing-main-4 Sep 19, 2024
16 checks passed
@jimit-j-shah jimit-j-shah deleted the jimit/remove_span_context branch September 19, 2024 17:14
jimit-j-shah added a commit that referenced this pull request Sep 19, 2024
* feat: Adding TraceUtil interface and its implementation to enable Tracing controls via DatastoreOptions (#1431)

* Adding EnabledTraceUtil, DisabledTraceUtil and TraceUtilTest
* Annotating DatastoreOpenTelemetryOptions to be transient as they're not serializable
* Adding google-auth-library-credentials dependency due to https://github.com/googleapis/java-datastore/actions/runs/8944472794/job/24571458116?pr=1431

* feat: Adding Lookup RPC OpenTelemetry Tracing (#1437)

* feat: Adding Lookup RPC OpenTelemetry Tracing
- Removed OpenCensus Tracing
- Added E2E tests with Global and Local OTel SDK
- Moved OTel SDK setup to RemoteDatastoreHelper
- Fixed pom to depend on BOM for all shared dependencies

* feat: Adding Commit RPC Trace Instrumentation (#1440)

- Added end-to-end test for Datastore operationsput, add, update and delete.
- Updated E2E Test to use the namespace correctly for efficient clean-up of test data

* feat: RunQuery trace instrumentation (#1441)

* feat: RunQuery trace instrumentation

* feat: RunAggregationQuery instrumentation (#1447)

* feat: RunQuery trace instrumentation

* Formatting

* Formatting

* Refactor: s/RUNQUERY/RUN_QUERY

* feat: RunAggregationQuery Trace Instrumentation

* Build: retiring test assertions for OpenCensus spans - will be replacing this in hermetic integration tests for OpenTelemetry using in-memory span exports (in addition to ITE2ETraceTest.java).

* Formatting

* Fixing @test annotation missed after merge

* Formatting

* feat: RunQuery trace instrumentation

* Formatting

* Formatting

* Refactor: s/RUNQUERY/RUN_QUERY

* feat: RunAggregationQuery Trace Instrumentation

* Build: retiring test assertions for OpenCensus spans - will be replacing this in hermetic integration tests for OpenTelemetry using in-memory span exports (in addition to ITE2ETraceTest.java).

* Formatting

* Fixing @test annotation missed after merge

* Formatting

* feat: Add Transaction tracing
test: transactionalLookupTest

* test: Transaction test for RunInTransaction - need to fix trace instrumentation for RunIn..

* Adding transaction span names

* TransactionLookupTest

* feat: support for transactional operations
- tested using newTransaction() and runInTransaction()

* Revert "feat: support for transactional operations"

This reverts commit 10341c0.

* feat: support for transactional operations (#1468)

* feat: support for transactional operations
- tested using newTransaction() and runInTransaction()

* feat: Allocateid tracing (#1488)

* feat: Adding tracing for AllocateIds RPC

* formatting

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

* feat: Add tracing for ReserveIds operation (#1490)

- added end-to-end test

* fix: Fixed Span nesting for `ReadWriteTransactionCallable` by using parent SpanContext instead of just parent Context (#1495)

* fix: Fixed the TraceUtil.startSpan method to use `SpanContext` for linking with the parent instead of `Context`.
- This fixes the hierarchy of Spans appearing in a transaction under a Run method.
- Tested using existing transaction test

* Fixed commit reordering and typos

* fix: lint errors

* fix: Refactored the ReadWriteTransactioncallable.call method to use startSpan idiomatically
- TraceUtil.startSpan needs more debugging
- return DefaultTracerProvider instance (no-op) when initializing DisabledTraceUtil - this fixes the unit tests in DatastoreTest.testRunInTransactionWithReadWriteOption

* feat: Added tracing for Transaction.RunQuery (#1499)

* feat: Added span for Transactional RunQuery
- tested

* fix: lint

* fix: patch apply issues

* fix: refactor using boolean flag

* fix: s/startSpan/startSpanWithParentContext

* test: Additional Transaction Testing and cleanup OpenCensus usage (#1505)

* test: newTransactionReadWriteTraceTest

* fix: test literal

* feat: Added tests for transaction cases

* fix: Cleanup OpenCensus dead code

* fix: updating version from 2.20.1 -> 2.21.0

* fix: reverting version from 2.21.0 -> 2.20.1

* fix: Adding an exception to the clirr-maven-plugin for an internal API parameter change from com.google.cloud.datastore.TraceUtil -> com.google.cloud.datastore.telemetry.TraceUtil

* fix: Fixing the differenceType in clirr exception

* fix: add an exception for removal of an internal class (com.google.cloud.datastore.TraceUtil)

* fix: fixing incomplete difference details for type 7005

* fix: Fixing `to` of the difference to be the entire signature

* fix: typo

* test: Adding ITTracingTest to verify events and span attributes (whic… (#1514)

* test: Adding ITTracingTest to verify events and span attributes (which are not verified in ITE2ETracingTest) due to TraceClient API limitations.
- This test uses InMemorySpanExporter to read the generated Otel span data by the test process to verify generated span data as it were before exporting to a backend. None of the span data is exported to a durable backend.
- This test is still an E2E test as it requires a project to send RPCs to.

* fix: fixing compilation error due to missing pom dependency.

* test: Test for AllocateId and ReserveId rpcs

* test: Commit/Put/Update/Delete tests

* test: Added fixes and test for RunQuery event

* test: Additional Transaction tests and AggregationQuery test (#1518)

* test: ReadWrite Transaction test

* test: Added test for Transactional RunQuery and Transaction Rollback

* test: runInTransaction API tracing test
- Fixed setting of common span attributes to spans in runInTransaction
- Removed some gRPC related channel attributes that are not present in this Datastore version, yet.

* fix: Undelete gRPC upgrade docs

* fix: Undo merge mistakes

* fix: Updating span event strings (#1539)

* fix: Fixing user-facing span names in line with go/firestore-client-trace-catalog

* fix: updating bom dependency version to fix https://github.com/googleapis/java-datastore/actions/runs/10256441634/job/28375496112?pr=1539

* Fix: typo in test causing integration test failure (#1556)

https://btx.cloud.google.com/invocations/c11a2e8b-4494-4ddc-a77e-cf2bcbcf5254/targets/cloud-devrel%2Fclient-libraries%2Fjava%2Fjava-datastore%2Fpresubmit%2Fintegration;config=default/log

* fix: opentelemetry-sdk should only be used as a Test Dependency

* fix: Update opentelemetry.version - this also fixes the tests failing in https://github.com/googleapis/java-datastore/actions/runs/10891578591/job/30222786908

* fix: Replacing attribute key values w/ constants

* fix: opentelemetry.version to fix RequireUpperBoundDeps check https://github.com/googleapis/java-datastore/actions/runs/10892403348/job/30225154043?pr=1576

* fix: Create Span hierarchy using parent Span (#1580)

* fix: Replace use of TraceUtil.SpanContext w/ TraceUtil.Context

* fix: Fixing how span hierarchy is created across threads - using Span instead of Context

* fix: cleaning up startSpan(spanName, parentContext) variant

* fix: add TracedReadWriteTransactionCallable to bifurcate tracing enabled/disabled paths for the Transaction callback.
- This change implements the idiomatic way to express nested spans as described in https://opentelemetry.io/docs/languages/java/instrumentation/#create-nested-spans

* fix: cleanup

* fix: cleanup

* fix: cleanup

* fix: formatting and import refactoring

* chore: generate libraries at Thu Sep 19 18:35:54 UTC 2024

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: cloud-java-bot <cloud-java-bot@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the googleapis/java-datastore API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants