beholder: send resource attributes to chip ingress as gRPC metadata - #2216
beholder: send resource attributes to chip ingress as gRPC metadata#2216pkcll wants to merge 12 commits into
Conversation
✅ API Diff Results -
|
e6eabcc to
fdb6dee
Compare
📊 API Diff Results
|
606a264 to
ada7f9b
Compare
| gopkg.in/yaml.v2 v2.4.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/smartcontractkit/chainlink-common/pkg/chipingress => ./pkg/chipingress |
There was a problem hiding this comment.
any way to avoid the local replace?
There was a problem hiding this comment.
For sure, will clean this up.
aa6b73c to
d6a531b
Compare
|
overall approach lgtm. Adding the CloudEvent extensions as the guaranteed propagation path makes sense, and we're keeping the gRPC header route for observability is reasonable |
2a6fa46 to
3ebb388
Compare
Pick up chip-ingress resource attribute propagation from smartcontractkit/chainlink-common#2216. Co-authored-by: Cursor <cursoragent@cursor.com>
e7e82a2 to
1f79a15
Compare
Pick up chip-ingress resource attribute propagation from smartcontractkit/chainlink-common#2216. Co-authored-by: Cursor <cursoragent@cursor.com>
Pick up chip-ingress resource attribute propagation from smartcontractkit/chainlink-common#2216. Co-authored-by: Cursor <cursoragent@cursor.com>
1f79a15 to
e1e0648
Compare
Pick up chip-ingress resource attribute propagation from smartcontractkit/chainlink-common#2216.
f5eda78 to
0a22a31
Compare
Pick up chip-ingress resource attribute propagation from smartcontractkit/chainlink-common#2216.
Pick up chip-ingress resource attribute propagation from smartcontractkit/chainlink-common#2216.
|
Split into two PRs:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
pkg/beholder/batch_emitter_service.go:26
- ChipIngressBatchEmitterService is an exported struct type; adding a
map[string]stringfield makes the type non-comparable, which can be an API-breaking change (similar to the concern noted in chip_ingress_emitter.go). Consider storing resource attributes behind a pointer wrapper (or another comparable container) to preserve type comparability, and only dereference it when stamping extensions.
type ChipIngressBatchEmitterService struct {
services.Service
eng *services.Engine
batchClient *batch.Client
resourceAttrs map[string]string
pkg/beholder/chip_ingress_emitter.go:64
- NewWithResourceAttributes stores the caller-provided
attrsmap directly. Because maps are mutable and not concurrency-safe, a caller mutating the map after construction (or concurrently with Emit) can cause data races or panics. Defensive-copy the map on entry before storing it on the emitter.
var resourceAttrs *resourceAttrExtensions
if len(attrs) > 0 {
resourceAttrs = &resourceAttrExtensions{attrs: attrs}
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
pkg/beholder/chip_ingress_emitter.go:64
- NewWithResourceAttributes stores the passed-in attrs map directly. Because maps are reference types, callers can mutate it after construction (or concurrently), which can change emitted events and potentially introduce data races. Consider defensively copying the map before storing it on the emitter.
var resourceAttrs *resourceAttrExtensions
if len(attrs) > 0 {
resourceAttrs = &resourceAttrExtensions{attrs: attrs}
}
pkg/beholder/batch_emitter_service.go:150
- emitInternal always uses NewEventWithOpts + WithResourceAttributeExtensions, even when no resource attributes are configured (resourceAttrs is an empty map). This adds overhead on every event and may inadvertently change behavior vs the previous NewEvent path. Consider only applying the opt when resourceAttrs is non-empty (mirroring ChipIngressEmitter.Emit).
attributes := newAttributes(attrKVs...)
event, err := chipingress.NewEventWithOpts(domain, entity, body, attributes, chipingress.WithResourceAttributeExtensions(e.resourceAttrs))
if err != nil {
return fmt.Errorf("failed to create CloudEvent: %w", err)
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pkg/beholder/chip_ingress_emitter.go:64
- NewWithResourceAttributes stores the caller-provided
attrsmap directly on the emitter. If the caller later mutates that map (or shares it across goroutines), emits can race on concurrent map reads/writes. Since these attributes are meant to be static config, defensively clone the map before storing it.
var resourceAttrs *resourceAttrExtensions
if len(attrs) > 0 {
resourceAttrs = &resourceAttrExtensions{attrs: attrs}
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/beholder/batch_emitter_service.go:147
- emitInternal always uses NewEventWithOpts + WithResourceAttributeExtensions, even when no resource attributes are configured (e.resourceAttrs is an empty map). This is inconsistent with the sync emitter and adds avoidable overhead / potential semantic differences when attrs are empty.
event, err := chipingress.NewEventWithOpts(domain, entity, body, attributes, chipingress.WithResourceAttributeExtensions(e.resourceAttrs))
| // resourceAttrExtensions holds resource attributes to stamp as CloudEvent extensions on every | ||
| // emitted event. It is stored behind a pointer on ChipIngressEmitter (rather than as a bare map | ||
| // field) so the struct itself stays a comparable type — a map field would make it incomparable, | ||
| // which is an exported-API-breaking change per apidiff. A nil *resourceAttrExtensions means no | ||
| // resource attributes are configured. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
pkg/beholder/batch_emitter_service.go:27
- Adding a map field makes ChipIngressBatchEmitterService no longer comparable. Comparability is part of the exported type’s API surface (and is enforced by api-diff in this repo), similar to the rationale noted in chip_ingress_emitter.go. To preserve comparability, store resource attributes behind a pointer wrapper (nil meaning “not configured”).
batchClient *batch.Client
resourceAttrs map[string]string
pkg/beholder/batch_emitter_service.go:99
- Resource attributes are currently stored as a (possibly empty) map and always passed through WithResourceAttributeExtensions. With an empty map, this still allocates/sorts on every emit (sanitizeResourceAttributeKeys). If no resource attrs are configured, skip the EventOpt entirely by keeping resourceAttrs nil and falling back to chipingress.NewEvent.
e := &ChipIngressBatchEmitterService{
batchClient: batchClient,
resourceAttrs: resourceAttributesToStringMap(cfg.ResourceAttributes),
metrics: metrics,
}
pkg/beholder/batch_emitter_service.go:151
- emitInternal unconditionally applies WithResourceAttributeExtensions, which does extra work even when no resource attributes are configured. Once resourceAttrs is stored as nil when unset, this can branch to NewEvent (no opts) to avoid per-event sanitization/sorting overhead.
attributes := newAttributes(attrKVs...)
event, err := chipingress.NewEventWithOpts(domain, entity, body, attributes, chipingress.WithResourceAttributeExtensions(e.resourceAttrs))
if err != nil {
return fmt.Errorf("failed to create CloudEvent: %w", err)
}
pkg/beholder/chip_ingress_emitter.go:64
- NewWithResourceAttributes stores the provided attrs map directly. Since this is an exported API, callers could mutate the map after construction, which can cause data races or panics when emitting (concurrent map read/write). Consider defensively cloning the map before storing it.
var resourceAttrs *resourceAttrExtensions
if len(attrs) > 0 {
resourceAttrs = &resourceAttrExtensions{attrs: attrs}
}
| func resourceAttributesToStringMap(attrs []attribute.KeyValue) map[string]string { | ||
| m := make(map[string]string, len(attrs)) | ||
| for _, kv := range attrs { | ||
| m[string(kv.Key)] = kv.Value.Emit() | ||
| } | ||
| return m | ||
| } |
Resource attributes from [Telemetry.ResourceAttributes] reached the OTel collector path but never the ChipIngress path, so they appeared as headers on beholder__platform__messages and not on cre. Wire them into the ChipIngress client. They travel once per request as gRPC metadata rather than being stamped on every CloudEvent. They describe the producer, not any individual event, which is why OTLP factors resource out of the payload rather than repeating it per log record; per-event stamping would also repeat roughly 300 KB of identical bytes for a batch of a thousand events with ten attributes, counting against maxGRPCRequestSize and reducing how many events fit per batch. Chip-ingress already fans connection-scoped values onto every Kafka record this way — nopInfoHeadersFromContext feeds baseKafkaHeaders, cached per (domain, entity, specVersion) — so this follows an existing server pattern. resourceAttributesToStringMap is the single conversion point, using attribute.Value.Emit for canonical stringification of any value type. Adds a regression test asserting on a real gRPC connection that configuring AuthHeaders together with ResourceAttributes leaves the CSA node auth token intact and delivers the attributes alongside it. The token travels as per-RPC credentials while attributes travel through a unary interceptor, so nothing previously covered the two mechanisms coexisting. No exported API is added or changed in pkg/beholder. This does not yet reach a Kafka consumer: chip-ingress reads incoming metadata only to authenticate, and forwarding it onto records is a server-side change still to come. Until that lands the attributes are visible to the service but not to consumers, so this does not on its own close the header gap between cre and beholder__platform__messages.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pkg/beholder/resource_attributes.go:8
- The doc comment claims this helper is the single source of truth for both gRPC metadata headers and CloudEvent extension keys/values, but in the current codebase it’s only used to derive gRPC metadata headers (the only call site is in client.go). This is misleading for future maintainers; either wire the CloudEvent extension propagation here as well or adjust the comment to match current behavior.
// resourceAttributesToStringMap converts OTel resource attributes into a plain string map,
// using attribute.Value.Emit for canonical stringification of any value type. This is the
// single source of truth used to derive both the gRPC metadata headers and the CloudEvent
// extension keys/values sent to ChipIngress, so both mechanisms stay consistent.
|
This PR is stale because it has been open 30 days with no activity. |
The comment described a CloudEvent-extension path that beholder never actually calls; resource attributes only ever reach ChipIngress as gRPC metadata headers.
Summary
Resource attributes set in node TOML (
[Telemetry.ResourceAttributes]) reach the OTel collectorpath but never the ChipIngress path, so they appear as headers on
beholder__platform__messagesand not on
cre. This wires them into the ChipIngress client.They travel once per request as gRPC metadata, not stamped on every CloudEvent. Resource
attributes describe the producer rather than any individual event — the same reason OTLP factors
resourceout of the payload instead of repeating it on every log record. Per-event stamping wouldalso repeat identical bytes for every event in a batch: ~10 attributes across a 1,000-event batch is
roughly 300 KB of duplication, which counts against
maxGRPCRequestSizeand therefore reduces howmany events fit per batch.
This also matches an existing chip-ingress pattern.
nopInfoHeadersFromContextalready reads aconnection-scoped value once and
baseKafkaHeadersfans it onto every Kafka record, cached per(domain, entity, specVersion).Changes
pkg/beholder/client.go— pass resource attributes to the ChipIngress client as gRPC metadata viachipingress.WithResourceAttributeHeaders.pkg/beholder/resource_attributes.go— addresourceAttributesToStringMap, the single conversionpoint, using
attribute.Value.Emitfor canonical stringification of any value type. (Its doccomment is worded to make explicit that this feeds gRPC metadata only — resource attributes are
never stamped as CloudEvent extensions.)
pkg/beholder/resource_attributes_test.go— unit coverage forresourceAttributesToStringMap,including the empty-input case.
pkg/beholder/chip_ingress_emitter.go— doc comment onChipIngressEmittermaking the same point:resource attributes are not stamped on individual events here, they travel once per request as
client-level gRPC metadata. Plus an incidental
gofmtalignment fix.pkg/beholder/batch_emitter_service_test.go— incidental stray-blank-linegofmtfix.pkg/beholder/client_test.go— regression test asserting on a real gRPC connection thatconfiguring
AuthHeaderstogether withResourceAttributesleaves the CSA node auth token intactand delivers the attributes alongside it.
go.mod/go.sum— bumppkg/chipingress.No exported API is added or changed in
pkg/beholder.Why the auth test
The CSA node auth token (
X-Beholder-Node-Auth-Token) travels ascredentials.PerRPCCredentials,while resource attributes travel through a unary interceptor that calls
metadata.AppendToOutgoingContext. Nothing previously covered the two coexisting. Two propertiesmatter and both are asserted:
collision would send two values under one key rather than overwriting
newClientStream, so a single non-printableattribute value would fail the entire RPC with
codes.Internal, auth included.pkg/chipingress'sSanitizeMetadataHeadersprevents this upstream by omitting an attribute with an invalid key or anon-printable value rather than sending it malformed — see chipingress: send resource attributes as prefixed gRPC metadata #2288.
Not yet visible to consumers
Chip-ingress currently reads incoming gRPC metadata only to authenticate
(
metadata.FromIncomingContextappears solely ininternal/auth/csa_auth.go). Kafka headers comefrom the CloudEvent's extensions and core attributes plus a fixed set the server derives itself.
So this PR makes the attributes visible to the service but not to a Kafka consumer. Forwarding
them onto records is smartcontractkit/atlas#13201 (open, draft, implemented and passing its own
suite), which requires a
resource_prefix inbound and emits the key unchanged — keeping thenamespace closed so a resource attribute cannot shadow
ce_*,table_route_name, or an identityheader the server derives from the verified auth token.
On its own, therefore, this PR does not close the header gap between
creandbeholder__platform__messages. It is one of three parts: this, #2288, and atlas#13201.Depends on #2288
The publisher-side prefixing, validation, and limits live in #2288, which namespaces every emitted
metadata key under
resource_, replaces character-rewriting with validate-and-omit for invalid keysand non-printable values, caps attributes at 32 entries / 128B key / 512B value / 4096B combined, and
warns + meters every attribute it has to drop. This PR needs no functional code change for any of
that: it calls
WithResourceAttributeHeaders, so all of it happens insidepkg/chipingress.One test-only fix is required at pin-bump time.
SanitizeMetadataHeaderschanged signature in#2288, from
func(map[string]string) map[string]stringtofunc(map[string]string) (map[string]string, []DroppedAttribute). This PR'sTestNewGRPCClient_AuthHeaderCoexistsWithResourceAttributes(pkg/beholder/client_test.go) currentlycalls it as a single-return function:
That compiles today only because this PR still pins the pre-#2288
pkg/chipingress(see below). Oncethe pin bumps to a post-#2288 commit, this call site needs updating to
want, _ := chipingress.SanitizeMetadataHeaders(...)before the module builds.It also needs the
pkg/chipingresspin bumped once #2288 merges. Until then theValidate go.mod dependenciesjob is expected to fail, as it has throughout this stack.Consumers reading
platformEnvtoday should move toresource_platformenv. They already need changesfor two other headers on these topics regardless:
beholder_entityisce_type, andcsa_public_keyis
ce_csapublickey— auth-derived, so more trustworthy than the self-reported value on the legacytopic.
Related
Related PRs
primitives this PR depends on)
chip-ingress server; forwards the metadata this PR sends onto Kafka headers; does not depend on
this PR, only on chipingress: send resource attributes as prefixed gRPC metadata #2288's wire contract)
Test plan