Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

database: revert to official github.com/XSAM/otelsql#47429

Merged
bobheadxi merged 2 commits into
mainfrom
02-05-database_revert_to_official_github.com/XSAM/otelsql
Feb 21, 2023
Merged

database: revert to official github.com/XSAM/otelsql#47429
bobheadxi merged 2 commits into
mainfrom
02-05-database_revert_to_official_github.com/XSAM/otelsql

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Feb 7, 2023

Copy link
Copy Markdown
Member

This is enabled by https://github.com/sourcegraph/sourcegraph/pull/47428 - swapping SpanProcessor instead of TracerProvider means that we can correctly propagate configuration changes to Tracer instances, so that we no longer need XSAM/otelsql#115 and can revert back to upstream. Switching to upstream, which has moved considerably since we forked it, necessitates a few other OpenTelemetry-related dependency upgrades as well (split out from https://github.com/sourcegraph/sourcegraph/pull/47126)

Whether updates are work is tested in https://github.com/sourcegraph/sourcegraph/pull/47428

Test plan

Search with ?trace=1 still has SQL traces with args:

Screenshot 2023-02-07 at 4 27 57 PM

Screenshot 2023-02-07 at 4 27 57 PM

@cla-bot cla-bot Bot added the cla-signed label Feb 7, 2023
@bobheadxi

Copy link
Copy Markdown
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@bobheadxi bobheadxi force-pushed the tracer-only-swap-processor branch from ba84992 to ee33483 Compare February 7, 2023 15:04
@bobheadxi bobheadxi force-pushed the 02-05-database_revert_to_official_github.com/XSAM/otelsql branch from 05d208a to 10e014e Compare February 7, 2023 15:05
@bobheadxi bobheadxi force-pushed the tracer-only-swap-processor branch from ee33483 to c9e6835 Compare February 7, 2023 15:07
@bobheadxi bobheadxi force-pushed the 02-05-database_revert_to_official_github.com/XSAM/otelsql branch 2 times, most recently from 0974c15 to d7c4bab Compare February 7, 2023 15:29
@bobheadxi bobheadxi marked this pull request as ready for review February 7, 2023 15:30
@bobheadxi bobheadxi requested a review from jhchabran February 7, 2023 15:30
@bobheadxi bobheadxi force-pushed the tracer-only-swap-processor branch from c9e6835 to e1fcc0e Compare February 7, 2023 15:41
@bobheadxi bobheadxi force-pushed the 02-05-database_revert_to_official_github.com/XSAM/otelsql branch 2 times, most recently from c0013a4 to a402f63 Compare February 7, 2023 16:31
@sourcegraph-bot

sourcegraph-bot commented Feb 7, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff e1fcc0ec3a2a05921515f12b3df2872468b74491...a402f631c7db02dd2e557b4d2d398c945be482eb.

Notify File(s)
@efritz internal/database/dbconn/open.go

@bobheadxi bobheadxi requested a review from burmudar February 10, 2023 08:46
@bobheadxi bobheadxi force-pushed the tracer-only-swap-processor branch from e1fcc0e to e91382e Compare February 13, 2023 16:53
@bobheadxi bobheadxi force-pushed the 02-05-database_revert_to_official_github.com/XSAM/otelsql branch from a402f63 to 2fce387 Compare February 13, 2023 16:53
@bobheadxi bobheadxi force-pushed the tracer-only-swap-processor branch from 793efb8 to 27f3961 Compare February 15, 2023 17:24
@bobheadxi bobheadxi force-pushed the 02-05-database_revert_to_official_github.com/XSAM/otelsql branch from 2fce387 to d031494 Compare February 15, 2023 17:24
@bobheadxi bobheadxi force-pushed the tracer-only-swap-processor branch from 27f3961 to c2a839a Compare February 21, 2023 17:14
@bobheadxi bobheadxi force-pushed the 02-05-database_revert_to_official_github.com/XSAM/otelsql branch from d031494 to 7c60078 Compare February 21, 2023 17:14
Base automatically changed from tracer-only-swap-processor to main February 21, 2023 17:39
bobheadxi referenced this pull request Feb 21, 2023
…#47428)

The new architecture of `internal/tracer`:

1. Creates a `TracerProvider` immediately. This provider is
preconfigured with service metadata, but does not have a
`SpanProcessor`.
2. Creates an OpenTracing bridge using `TracerProvider`, such that we
have an OpenTracing Tracer and an OpenTelemetry TracerProvider that
bridges the two APIs. We register both as global defaults.
3. On site config initialization, we register a new `SpanProcessor` onto
the global `TracerProvider`.
4. From this point on, site config updates:
   a. Create a new `SpanProcessor` based on site config
   b. Register the new `SpanProcessor`, which starts processing
c. Remove the previous `SpanProcessor`, which flushes it and shuts it
down as well

Previously, we mimicked the old opentracing setup where we swapped the
global tracer, using a similar approach in OpenTelemetry to set up the
global TracerProvider. In OpenTelemetry, registering and unregistering
`SpanProcessors` is the correct way to do this it seems - the swapping
approach causes configuration propagation issues such as
XSAM/otelsql#115 (allowing us to revert back to
upstream: https://github.com/sourcegraph/sourcegraph/pull/47429) and
possibly https://github.com/sourcegraph/sourcegraph/issues/42515 . This
correct approach was pointed out in
XSAM/otelsql#115 (comment), and
makes the setup easier to reason with:

- the previously swappable tracers now no longer need to have a locked
underlying implementation, they just add logging now
- we don't need to update 2 implementations, we make the update at a
lower level so that both ends of the OT/OTel bridge are updated
- debug mode is managed through a mutable atomic bool, instead of
through recreating entire tracers

## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->

Unit tests assert that span processor updates are propagated. Exporter
setup is functionally unchanged

---------

Co-authored-by: Jean-Hadrien Chabran <jh@chabran.fr>
@bobheadxi bobheadxi force-pushed the 02-05-database_revert_to_official_github.com/XSAM/otelsql branch from 7c60078 to 6ad66cf Compare February 21, 2023 17:39
@bobheadxi bobheadxi merged commit a071a07 into main Feb 21, 2023
@bobheadxi bobheadxi deleted the 02-05-database_revert_to_official_github.com/XSAM/otelsql branch February 21, 2023 19:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants