kube-rs / controller-rs

A kubernetes reference controller
Apache License 2.0
270 stars 28 forks source link

Add manual tests for valid TraceIds #45

Closed clux closed 1 year ago

clux commented 1 year ago

to avoid further confusion for stuff like #44. sets up a minimal context to verify that we get a trace. saved under a shortcut for just test-telemetry that requires something like just forward-tempo to work.

can be made fully automated (see below), but it requires a valid tempo instance on CI or some mocking setup for tracers that does not produce invalid traceids (afaikt the mock tracers all produce zeroes).

clux commented 1 year ago

hm. i think the bug is actually invalid; it "works for me" when using just run-telemetry having port-forwarded to a local tempo instance.

clux commented 1 year ago

doesn't look like i can test this locally without a full tracer. even their noop tracers return zeroes:

impl PreSampledTracer for noop::NoopTracer {
    fn sampled_context(&self, data: &mut crate::OtelData) -> OtelContext {
        data.parent_cx.clone()
    }

    fn new_trace_id(&self) -> otel::TraceId {
        otel::TraceId::INVALID
    }

    fn new_span_id(&self) -> otel::SpanId {
        otel::SpanId::INVALID
    }
}
clux commented 1 year ago

ugh, i'd have to basically install tempo into the cluster for the integration tests using the chart with some very simple values and helm using something like:

      - uses: azure/setup-helm@v3
      - run: helm install grafana/tempo --wait -f values.yaml
      - run: just forward-tempo
      - run: just test-telemetry

with chart values.yaml as something like:

replicas: 1
tempo:
  retention: 24h
  authEnabled: false
  server:
    http_listen_port: 3100
  storage:
    trace:
      backend: local
      local:
        path: /var/tempo/traces
      wal:
        path: /var/tempo/wal
  receivers:
    otlp:
      protocols:
        http:
          endpoint: "0.0.0.0:55681"
        grpc:
          endpoint: "0.0.0.0:4317"
    jaeger:
      protocols:
        grpc:
          endpoint: "0.0.0.0:14250"
persistence:
  enabled: false

(this works in my old tempo pin at 1.0.1, but probably needs changes)

i'll leave this to a separate PR and leave the test as manual for now.