fluxninja / aperture

Rate limiting, caching, and request prioritization for modern workloads
https://docs.fluxninja.com
Apache License 2.0
642 stars 25 forks source link

Docs on flow label `telemetry` flag inconsistent with implementation #2183

Closed krdln closed 1 year ago

krdln commented 1 year ago

All flow labels have the telemetry flag steering whether flow label should be included in [ARC] telemetry.

https://github.com/fluxninja/aperture/blob/71957f6293e4d5cff0cd81ce4f92f2962e1e95c8/pkg/policies/flowcontrol/label/flow-label.go#L14-L18

This is togglable for classifier-created flow labels and also, we set this flag for builtin flow labels: https://github.com/fluxninja/aperture/blob/71957f6293e4d5cff0cd81ce4f92f2962e1e95c8/pkg/policies/flowcontrol/service/checkhttp/checkhttp-labels.go#L30-L65

The implementation though ignores this flag and behaves as if all flow labels (classifier-created and builtin) had telemetry: true.

https://github.com/fluxninja/aperture/blob/71957f6293e4d5cff0cd81ce4f92f2962e1e95c8/pkg/policies/flowcontrol/service/checkhttp/checkhttp.go#L163

https://github.com/fluxninja/aperture/blob/71957f6293e4d5cff0cd81ce4f92f2962e1e95c8/pkg/policies/flowcontrol/service/checkhttp/checkhttp.go#L176

(note the ToPlainMap – it drops the telemetry flag). https://github.com/fluxninja/aperture/blob/71957f6293e4d5cff0cd81ce4f92f2962e1e95c8/pkg/policies/flowcontrol/label/flow-label.go#L33

Either docs are outdated or impl wrong.

krdln commented 1 year ago

Either docs are outdated or impl wrong.

Note: There's some code duplication between checkhttp & authz, make sure to handle both places!

krdln commented 1 year ago

Update: The consensus is that we should stop sending header values. In other words: the Telemetry: false should always be respected.