internal/trace: remove net/trace integration by bobheadxi · Pull Request #53795 · sourcegraph/sourcegraph-public-snapshot

bobheadxi · 2023-06-20T19:13:49Z

Follow-up from https://github.com/sourcegraph/sourcegraph/pull/53435#issuecomment-1597050970 and INC-214 - this removes net/trace instrumentation entirely in the next release.

It seems this gets low usage and may be difficult to use, and it seems to introduce some risk and overhead. This will also make it easier to drop our internal trace wrapper entirely and/or integrate it as part of OpenTelemetry's APIs instead of having a proprietary way of doing traces.

Test plan

Already tested in https://github.com/sourcegraph/sourcegraph/pull/53435

sourcegraph-bot · 2023-06-20T19:15:21Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff f989f73...39e20e8.

Notify	File(s)
@keegancsmith	internal/debugserver/BUILD.bazel internal/debugserver/debug.go internal/trace/BUILD.bazel internal/trace/trace.go internal/trace/tracer.go
@sourcegraph/delivery	doc/admin/observability/tracing.md
@sourcegraph/dev-experience	internal/trace/BUILD.bazel internal/trace/trace.go internal/trace/tracer.go

camdencheek

Is there a way we can set this somewhere in the UI (site settings maybe) rather than with an env var? Getting changes to deployments can be a pain, so this will be tough to use if we need customers to update their config and redeploy

bobheadxi · 2023-06-20T19:26:02Z

Is there a way we can set this somewhere in the UI (site settings maybe) rather than with an env var? Getting changes to deployments can be a pain, so this will be tough to use if we need customers to update their config and redeploy

Was thinking this - we can set a context and global thing a la trace policy. Will take that for a spin

keegancsmith

I'd consider just ripping the code out TBH. Its so broken right now given how it integrates with all tracing that turning it on doesn't help. Last few times I tried to use it wasn't fun. There is a possibility someone relies on it for customers more regularly, but its unlikely.

cc @DaedalusG do you or anyone else in your crew ever use these endpoints? I checked and we don't actually document this as a way to get insights.

DaedalusG · 2023-06-21T07:05:21Z

@keegancsmith I think net/trace is rarely if ever used. Jaeger traces are the most commonly used tracing. Sometimes the /debug/ page is used but its inconsistent across deployments so its rarely reached for first. I hate to rip out any observability but if its causing memory leaks it can probably go.

Last few times I tried to use it wasn't fun.

I try to avoid exposing this to customers for this reason. I think maybe some of our older admins and CE's maybe use it.

bobheadxi · 2023-06-21T16:17:59Z

Alright - let's remove it entirely then :)

bobheadxi · 2023-06-21T16:31:39Z

Updated the PR to remove net/trace entirely, PTAL! @camdencheek this might make it easier to try and drop our internal/trace interface entirely as well

camdencheek · 2023-06-21T16:35:10Z

integrate it as part of OpenTelemetry's APIs

FWIW, I spiked out an implementation that bridges x/net/trace with the OTel interfaces, so we could add this back in the future if we wanted even if we move to more raw OpenTelemetry APIs

camdencheek · 2023-06-21T16:36:02Z


 ### net/trace

+> WARNING: `net/trace` instrumentation will be removed in Sourcegraph 5.2.


should we remove this in 5.1 if we're concerned about stability issues?

Hmm, yeah let's do that, especially if it's feasible to add back if needed

Follow-up from https://github.com/sourcegraph/sourcegraph/pull/53435#issuecomment-1597050970 and INC-214 - this removes `net/trace` instrumentation entirely in the next release. It seems [this gets low usage and may be difficult to use](https://github.com/sourcegraph/sourcegraph/pull/53795#pullrequestreview-1489890896), and it seems to introduce some risk and overhead. This will also make it easier to drop our internal trace wrapper entirely and/or integrate it as part of OpenTelemetry's APIs instead of having a proprietary way of doing traces. ## Test plan Already tested in https://github.com/sourcegraph/sourcegraph/pull/53435 (cherry picked from commit 61ce005)

Follow-up from https://github.com/sourcegraph/sourcegraph/pull/53435#issuecomment-1597050970 and INC-214 - this removes `net/trace` instrumentation entirely in the next release. It seems [this gets low usage and may be difficult to use](https://github.com/sourcegraph/sourcegraph/pull/53795#pullrequestreview-1489890896), and it seems to introduce some risk and overhead. This will also make it easier to drop our internal trace wrapper entirely and/or integrate it as part of OpenTelemetry's APIs instead of having a proprietary way of doing traces. ## Test plan Already tested in https://github.com/sourcegraph/sourcegraph/pull/53435

Follow-up from https://github.com/sourcegraph/sourcegraph/pull/53435#issuecomment-1597050970 and INC-214 - this removes `net/trace` instrumentation entirely in the next release. It seems [this gets low usage and may be difficult to use](https://github.com/sourcegraph/sourcegraph/pull/53795#pullrequestreview-1489890896), and it seems to introduce some risk and overhead. This will also make it easier to drop our internal trace wrapper entirely and/or integrate it as part of OpenTelemetry's APIs instead of having a proprietary way of doing traces.

Follow-up from https://github.com/sourcegraph/sourcegraph/pull/53435#issuecomment-1597050970 and INC-214 - this removes `net/trace` instrumentation entirely in the next release. It seems [this gets low usage and may be difficult to use](https://github.com/sourcegraph/sourcegraph/pull/53795#pullrequestreview-1489890896), and it seems to introduce some risk and overhead. This will also make it easier to drop our internal trace wrapper entirely and/or integrate it as part of OpenTelemetry's APIs instead of having a proprietary way of doing traces. ## Test plan Already tested in https://github.com/sourcegraph/sourcegraph/pull/53435

bobheadxi requested review from a team, camdencheek and keegancsmith June 20, 2023 19:13

cla-bot Bot added the cla-signed label Jun 20, 2023

internal/trace: disable net/trace by default

5c27fbf

bobheadxi force-pushed the trace-disable-net-trace-by-default branch from 580d308 to 5c27fbf Compare June 20, 2023 19:14

camdencheek reviewed Jun 20, 2023

View reviewed changes

bobheadxi marked this pull request as draft June 20, 2023 19:26

keegancsmith approved these changes Jun 21, 2023

View reviewed changes

remove net/trace entirely

d706e2f

bobheadxi changed the title ~~internal/trace: disable net/trace by default~~ internal/trace: remove net/trace integration Jun 21, 2023

bobheadxi requested review from camdencheek and keegancsmith June 21, 2023 16:31

bobheadxi marked this pull request as ready for review June 21, 2023 16:31

camdencheek reviewed Jun 21, 2023

View reviewed changes

camdencheek approved these changes Jun 21, 2023

View reviewed changes

remove net/trace now

39e20e8

bobheadxi added the backport 5.1 label Jun 21, 2023

camdencheek approved these changes Jun 21, 2023

View reviewed changes

bobheadxi enabled auto-merge (squash) June 21, 2023 16:51

bobheadxi merged commit 61ce005 into main Jun 21, 2023

bobheadxi deleted the trace-disable-net-trace-by-default branch June 21, 2023 18:35

github-actions Bot mentioned this pull request Jun 21, 2023

[Backport 5.1] internal/trace: remove net/trace integration #53889

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/trace: remove net/trace integration#53795

internal/trace: remove net/trace integration#53795
bobheadxi merged 3 commits into
mainfrom
trace-disable-net-trace-by-default

bobheadxi commented Jun 20, 2023 •

edited

Loading

Uh oh!

sourcegraph-bot commented Jun 20, 2023 •

edited

Loading

Uh oh!

camdencheek left a comment •

edited

Loading

Uh oh!

bobheadxi commented Jun 20, 2023

Uh oh!

keegancsmith left a comment

Uh oh!

DaedalusG commented Jun 21, 2023 •

edited

Loading

Uh oh!

bobheadxi commented Jun 21, 2023

Uh oh!

bobheadxi commented Jun 21, 2023

Uh oh!

camdencheek commented Jun 21, 2023

Uh oh!

camdencheek Jun 21, 2023

Uh oh!

bobheadxi Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants


		### net/trace

		> WARNING: `net/trace` instrumentation will be removed in Sourcegraph 5.2.

Conversation

bobheadxi commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

sourcegraph-bot commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camdencheek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobheadxi commented Jun 20, 2023

Uh oh!

keegancsmith left a comment

Choose a reason for hiding this comment

Uh oh!

DaedalusG commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobheadxi commented Jun 21, 2023

Uh oh!

bobheadxi commented Jun 21, 2023

Uh oh!

camdencheek commented Jun 21, 2023

Uh oh!

camdencheek Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

bobheadxi Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bobheadxi commented Jun 20, 2023 •

edited

Loading

sourcegraph-bot commented Jun 20, 2023 •

edited

Loading

camdencheek left a comment •

edited

Loading

DaedalusG commented Jun 21, 2023 •

edited

Loading