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.

Sign up for GitHub

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

Jump to bottom

opentelemetry: upgrade go.opentelemetry.io libraries #47126

Merged
merged 14 commits into from
Feb 23, 2023
Merged

opentelemetry: upgrade go.opentelemetry.io libraries #47126

merged 14 commits into from
Feb 23, 2023

Conversation

bobheadxi
Copy link
Member

@bobheadxi bobheadxi commented Jan 30, 2023 •
edited
Loading

Upgrades all go.opentelemetry.io and @opentelemetry/ libraries, both in the web app and in the backend, to the latest versions. This change also refactors client OTEL pass-through to work with the breaking changes in collector-internal packages required for the pass-through.

unblocks #40945 (at last!!!)

Test plan

CI passes

Manual testing of the client-side passthrough with sg start and sg start monitoring:

https://sourcegraph.test:3443/search?q=context:global+foobar&patternType=standard&sm=1&trace=1&groupBy=repo

Screenshot 2023-02-21 at 12 53 44 PM

Random web-app trace is forwarded to Jaeger correctly, and attaches to backend spans as before:

Screenshot 2023-02-21 at 1 04 18 PM

@cla-bot cla-bot bot added the cla-signed label Jan 30, 2023
@bobheadxi
Copy link
Member Author

@jhchabran I believe this is blocked also on XSAM/otelsql#115 - the fork needs to be updated for the new metrics API

@bobheadxi
Copy link
Member Author

bobheadxi commented Feb 2, 2023 •
edited
Loading

Now blocked on https://github.com/open-telemetry/opentelemetry-collector cutting the next release

update - https://github.com/open-telemetry/opentelemetry-collector/releases/tag/cmd%2Fbuilder%2Fv0.71.0 now available

@jhchabran
Copy link
Member

@jhchabran I believe this is blocked also on XSAM/otelsql#115 - the fork needs to be updated for the new metrics API

@bobheadxi Ok, I'll take a look at this this week, regardless of what we do, it shouldn't block the outcome (I can always backport our patch).

@jhchabran jhchabran self-assigned this Feb 6, 2023
@bobheadxi
Copy link
Member Author

@jhchabran I hacked on the approach the maintainer of otelsql mentioned while on my flight, it seems to work and simplifies our tracer package a bit! This might be the root cause of other configuration issues we've seen as well, will put up a PR this week

@jhchabran
Copy link
Member

@bobheadxi Ack! Good news then 😊 Feel free to ping me on the PR for a review !

@bobheadxi
Copy link
Member Author

bobheadxi commented Feb 7, 2023 •
edited
Loading

See: #47428 and #47429

@bobheadxi bobheadxi mentioned this pull request Feb 7, 2023
Merged
bobheadxi added a commit that referenced this pull request Feb 21, 2023
@bobheadxi
database: revert to official github.com/XSAM/otelsql (#47429)
a071a07 
This is enabled by #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
#47126)

Whether updates are work is tested in
#47428

## Test plan

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

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

<img width="1649" alt="Screenshot 2023-02-07 at 4 27 57 PM"
src="https://user-images.githubusercontent.com/23356519/217288396-3069e947-1841-4108-8ab6-02a384a98a54.png">

<img width="1649" alt="Screenshot 2023-02-07 at 4 27 57 PM"
src="https://user-images.githubusercontent.com/23356519/217288612-f5091180-a2f3-4144-ab6f-3456cc187612.png">
@bobheadxi bobheadxi marked this pull request as ready for review February 21, 2023 21:06
@bobheadxi
Copy link
Member Author

@jhchabran with #47428 and #47429 merged, I updated this PR and tested it end-to-end locally, and everything seems to be working - should now be ready for review :) also cc @valerybugakov as this required some updates of the clientside OTEL libraries

@sg-e2e-regression-test-bob
Copy link

sg-e2e-regression-test-bob commented Feb 21, 2023 •
edited
Loading

Bundle size report 📦

Initial size Total size Async size Modules
-2.29% (-64.09 kb) 🔽 -0.43% (-64.11 kb) 🔽 -0.00% (-0.01 kb) 0.26% (+2) 🔺

Look at the Statoscope report for a full comparison between the commits c5021cf and dc9d370 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@bobheadxi bobheadxi mentioned this pull request Feb 21, 2023
Merged
bobheadxi
bobheadxi commented Feb 21, 2023
View reviewed changes
cmd/frontend/internal/app/otlpadapter/register.go
return metric.NewNoopMeterProvider()
}
return sdkmetric.NewMeterProvider(sdkmetric.WithReader(prom))
}
Copy link
Member Author

Choose a reason for hiding this comment

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

might be of interest @jhchabran - this seems to be the finalized OTEL metrics API.

bobheadxi
bobheadxi commented Feb 21, 2023
View reviewed changes
cmd/frontend/internal/app/otlpadapter/register.go
Comment on lines +51 to +52
MeterProvider: mustPrometheusMetricsProvider(logger),
MetricsLevel: configtelemetry.LevelBasic,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is new and required - enabled in Prometheus so that we might experiment with it. Locally, however, I haven't noticed any metrics emitted by this (but I didn't dig too deep)

@bobheadxi bobheadxi enabled auto-merge (squash) February 23, 2023 15:53
@bobheadxi bobheadxi merged commit 7f05787 into main Feb 23, 2023
@bobheadxi bobheadxi deleted the opentelemetry-upgrade branch February 23, 2023 17:03
bobheadxi added a commit that referenced this pull request Feb 23, 2023
@bobheadxi
internal/tracer: correctly set Tracer names in OpenTracing bridge (#4…
1cb5bc4 
…0945)

The existing wrapped OpenTelemetry TracerProvider for bridging
OpenTracing API calls to OpenTelemetryAPI calls discards the name
provided to the `Tracer()` constructor on it, since it uses a fixed
tracer that we provide it.

This change leverages an upstream patch,
open-telemetry/opentelemetry-go#3116 (dependency
update: #47126), that
allows wrapped tracers to be created on the fly with the provided
parameters, so that we can more accurately see the instrumentation
source.

## Test plan

Via the bridge, we can see that the Tracer used is the bridge tracer:

<img width="1322" alt="image"
src="https://user-images.githubusercontent.com/23356519/186957763-60faee32-e6a5-4bda-bb64-fddb82eacc0d.png">

Via direct usage of OpenTelemetry, we can see that the Tracer used has
the custom name set:

<img width="1448" alt="image"
src="https://user-images.githubusercontent.com/23356519/186957773-c6ff7643-3f89-498d-857b-1dd79b36508d.png">
@bobheadxi bobheadxi mentioned this pull request Feb 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhchabran jhchabran jhchabran approved these changes

@burmudar burmudar Awaiting requested review from burmudar

@valerybugakov valerybugakov Awaiting requested review from valerybugakov

Assignees

@jhchabran jhchabran

Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bobheadxi @jhchabran @sg-e2e-regression-test-bob