Conversation
There was a problem hiding this comment.
Pull request overview
Adds OpenTelemetry tracing to correlate a device’s proxy assignment with its first observed proxy connection, enabling connectivity success-rate and time-to-connect analysis in SigNoz.
Changes:
- Emit a
device_id.connectedspan when CLIENTINFO is received on routed TCP/packet connections. - Add OTLP/HTTP trace exporter initialization alongside existing metrics exporter initialization.
- Wire clientcontext manager into the run path by default and add a local ping script / Makefile flag plumbing.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tracker/clientcontext/manager.go |
Emits device_id.connected span with client attributes on successful CLIENTINFO parsing. |
otel/otel.go |
Adds InitGlobalTracerProvider using OTLP/HTTP exporter and SDK tracer provider. |
cmd/main.go |
Initializes global meter + tracer providers during CLI pre-run when proxy info is available. |
cmd/cmd_run.go |
Always registers the clientcontext manager (previously conditional) to enable tracing. |
cmd/Makefile |
Passes --telemetry-endpoint through make run. |
scripts/ping.sh |
Adds a script intended to exercise CLIENTINFO and produce the new span. |
go.mod / go.sum |
Adds OTLP trace HTTP exporter dependencies. |
.gitignore |
Ignores cmd/cmd. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Hey @garmr-ulfr, do you mind looking at this? The CLIENTINFO stuff is a little foreign to me, but running For context, this is my first draft adding spans to lantern-box for the purpose of better success/failure tracking after config assignment: It's been suggested this entire approach could be supplanted by using the strategy laid out in: |
garmr-ulfr
left a comment
There was a problem hiding this comment.
Besides ping.sh, LGTM. You implemented it correctly, but your test script is wrong.
Crosse
left a comment
There was a problem hiding this comment.
My only concern is that I believe we need to drop a bunch of our OTel configuration complexity and just use the built-in environment variables.
@Crosse sorry I missed it the first time you said this. Will address today. |
|
@jay-418 You didn't miss anything—I never hit Send on the review last week. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jay-418 I'm not sure I've mentioned this yet, but just FYI in case you weren't aware, lantern-box must be usable by anyone, so any changes shouldn't restrict it to Lantern. Not that you have, just a heads up 😄 |
Sure thing @garmr-ulfr. I just completed some heavier handed changes, but they make everything more generic by using the standard OTEL convetions for env vars. |
garmr-ulfr
left a comment
There was a problem hiding this comment.
LGTM! This will also require an update in lantern-cloud to use the --proxy-info flag once this is merged and deployed.
purpose
Provide mechanism for recording a
device_id's first connection to a route to provide an indication of poor network conditions.mechanism
Emit a span with
device_id(and some attrs) when a proxy is assigned by the API during config request. Then also emit a span when adevice_idfirst connects to a proxy.In SigNoz, we can calculate (using some kinda gnarly ClickHouse queries as discussed in https://github.com/getlantern/engineering/issues/2987):
refactor + breaking changes
Eliminate custom OTEL in favor of standard vars.
--telemetry-endpointflagOTEL_EXPORTER_OTLP_ENDPOINTCUSTOM_OTLP_ENDPOINTOTEL_EXPORTER_OTLP_ENDPOINTOTEL_EXPORTER_OTLP_INSECURE=truelantern-boxservice nameOTEL_SERVICE_NAMElantern-boxOTEL_RESOURCE_ATTRIBUTES.inifiledocker test dependency
Addition of "e2e" tests use docker and
testcontainersto simulate all parts: client, singbox, otel, and upstream. Bypass with-shortflag:go test ./... -short
semconv
Using standard KVs for otel, with updates getlantern/semconv@121098b