Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1712 +/- ##
==========================================
- Coverage 78.14% 78.13% -0.01%
==========================================
Files 278 279 +1
Lines 34240 34620 +380
==========================================
+ Hits 26758 27052 +294
- Misses 6229 6293 +64
- Partials 1253 1275 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1f87cbe to
e86822a
Compare
grcevski
left a comment
There was a problem hiding this comment.
I just had a few minor comments, but this is looking pretty good!
| return errors.New("invalid NATS sid") | ||
| } | ||
| for _, b := range field { | ||
| if (b < '0' || b > '9') && (b < 'a' || b > 'z') && (b < 'A' || b > 'Z') { |
There was a problem hiding this comment.
sql_detect_transform has the same code, maybe we can combine this into a common helper?
|
|
||
| return natsFrame{clientID: meta.Name, valid: true}, nil | ||
| case "SUB": | ||
| if len(fields) != 3 && len(fields) != 4 { |
There was a problem hiding this comment.
Nit: I found these statements confusing with != 3 && !=4. It might be better to write them as !(len==3 || len==4)
There was a problem hiding this comment.
this is how it was before, but linter is not happy:
Error: pkg/ebpf/common/nats_detect_transform.go:139:6: QF1001: could apply De Morgan's law (staticcheck)
if !(len(fields) == 3 || len(fields) == 4) {
| ))) | ||
| } | ||
|
|
||
| func TestParseNATSFrame(t *testing.T) { |
There was a problem hiding this comment.
Can we add more tests with all the variations of !=4 && !=5?
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for capturing NATS (plain TCP) messaging spans, including protocol detection/parsing in the eBPF TCP pipeline, wiring NATS into instrumentation selection/config/schema, and exporting NATS spans/metrics via Prometheus and OTEL (plus an OATS integration suite).
Changes:
- Add NATS protocol parsing/detection in the TCP userspace parser and map it to new NATS span event types.
- Export NATS spans/attributes and messaging publish/process duration metrics in both Prometheus and OTEL exporters.
- Add documentation + config schema updates + an OATS NATS integration test suite (Python client).
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/obi/config.go | Enable NATS in default instrumentation list. |
| pkg/obi/config_test.go | Update config tests to include NATS defaults. |
| pkg/export/prom/prom.go | Record messaging publish/process histograms for NATS spans. |
| pkg/export/prom/prom_test.go | Add Prom metrics expectations and span fixtures for NATS. |
| pkg/export/otel/tracesgen/tracesgen.go | Gate NATS spans on instrumentation selection; emit NATS messaging attributes; set span kinds. |
| pkg/export/otel/traces_test.go | Add NATS trace attribute test vectors and instrumentation filtering expectations. |
| pkg/export/otel/metrics.go | Record OTEL messaging publish/process metrics for NATS spans. |
| pkg/export/otel/metrics_test.go | Add NATS metric expectations and connection-type coverage. |
| pkg/export/otel/metrics_svc_graph_test.go | Include NATS in service graph connection type validation. |
| pkg/export/instrumentations/instr_options.go | Add InstrumentationNATS, selection flag, and NATSEnabled/MQEnabled logic. |
| pkg/export/instrumentations/instr_options_test.go | Add selection tests for NATSEnabled and MQEnabled behavior. |
| pkg/ebpf/common/tcp_detect_transform.go | Detect NATS traffic, parse it, and emit main + optional extra span. |
| pkg/ebpf/common/tcp_detect_transform_test.go | Add NATS parsing tests and coalesced publish+process extra-span emission test. |
| pkg/ebpf/common/nats_detect_transform.go | New NATS protocol parser, heuristic detection, and span conversion. |
| pkg/ebpf/common/nats_detect_transform_test.go | Unit tests for NATS heuristics, frame parsing, and event handling. |
| pkg/ebpf/common/common.go | Add protocol type placeholder and mark NATS client spans as client events. |
| pkg/appolly/app/request/span.go | Add NATS event types and integrate into span kind, naming, attrs, and service graph connection type. |
| pkg/appolly/app/request/span_test.go | Extend span tests for new NATS event types and behaviors. |
| pkg/appolly/app/request/span_getters.go | Add OTEL getter support for NATS messaging attributes. |
| pkg/appolly/app/request/span_getters_test.go | Add getter test coverage for NATS messaging attributes. |
| Makefile | Add oats-test-nats and include it in oats-test. |
| internal/test/oats/nats/yaml/oats_python_nats.yaml | OATS spec validating NATS spans + messaging duration metrics. |
| internal/test/oats/nats/oats_test.go | Register NATS OATS suite. |
| internal/test/oats/nats/go.mod | Go module for the NATS OATS suite. |
| internal/test/oats/nats/go.sum | Dependency lockfile for the NATS OATS suite. |
| internal/test/oats/nats/docker-compose-obi-python-nats.yml | Compose stack: NATS server + python test app + instrumenter. |
| internal/test/oats/nats/docker-compose-include-base.yml | Template include helper for OATS compose generation. |
| internal/test/oats/nats/docker-compose-generic-template.yml | Generic OATS infra stack template (grafana/prom/tempo/collector). |
| internal/test/oats/nats/configs/tempo-config.yaml | Tempo config for OATS NATS suite. |
| internal/test/oats/nats/configs/prometheus-config.yml | Prometheus config for OATS NATS suite. |
| internal/test/oats/nats/configs/otelcol-config.yaml | Collector pipeline for traces/metrics in OATS NATS suite. |
| internal/test/oats/nats/configs/instrumenter-config-traces.yml | Instrumenter routes config used by the suite. |
| internal/test/oats/nats/configs/grafana-datasources.yaml | Grafana datasources for the suite stack. |
| internal/test/integration/components/pythonnats/requirements.txt | Pinned python dependency (nats-py) for integration component. |
| internal/test/integration/components/pythonnats/requirements.in | Input dependency list for pip-compile. |
| internal/test/integration/components/pythonnats/main.py | Python app that publishes to NATS and serves a /publish endpoint. |
| internal/test/integration/components/pythonnats/Dockerfile | Container image for the python NATS test component. |
| docs/config-schema.json | Add nats to instrumentation enums in the config schema. |
| devdocs/protocols/tcp/README.md | Link NATS protocol parser documentation. |
| devdocs/protocols/tcp/nats.md | New documentation for the NATS TCP parser and limitations. |
| devdocs/features.md | Document NATS support in features matrix. |
rafaelroquetto
left a comment
There was a problem hiding this comment.
Overall, I think we should do another pass to split the code for clarity, and also ensure no unecessary string allocations are taking place (that's one of the reasons why I refactored the sql code when doing the large buffers API, so I would prefer if we did not drift from the approach).
It might be worth it doing a local review telling your agent to look into AGENTS.md - for some reason I think copilot (the github one) has been sloppy and missing a lot of obvious cases.
CI Supervisor
|
Summary
Add support to capture span from clients using NATS protocol.
Fixes #1143
Validation