Conversation
…ata bug structured_metadata mutates Entry.Labels in place, deleting a label once it's promoted to structured metadata. parseCWEvent and processLogEvents build one labels map per invocation and pass it by reference to every entry in a batch, so the delete from the first entry was visible to every later entry sharing that map -- only the first entry in a batch kept the field.
|
|
|
|
| processor: pipeline, | ||
| } | ||
|
|
||
| batchSize = 131072 // large enough that add() never flushes mid-test |
There was a problem hiding this comment.
What is the precedent for this magic number? Should it be based on a config setting or something?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok thanks, what's the precedent for that magic number 😆
Can they both be commented with reference to documentation etc or fixed to point to something that's easier to know the correctness of like a config setting?
| // events), parseCWEvent/processLogEvents build one labels map and reuse it, by reference, for | ||
| // every entry{} in the batch. The structured_metadata stage mutates Entry.Labels in place | ||
| // (deleting a label once it's promoted to structured metadata), so without batch.add cloning | ||
| // e.labels first, only the first entry in the batch keeps the field -- every later entry's |
There was a problem hiding this comment.
only the first entry in the batch keeps the field
Why would this be true if every entry{} in the batch shares the same reference? Wouldn't they all see the mutated map?
| // Test_batch_add_SharedLabelsAcrossBatch reproduces | ||
| // https://github.com/grafana/support-escalations/issues/24182: when a Lambda invocation | ||
| // receives multiple log events in one batch (e.g. a CloudWatch put-log-events call with several | ||
| // events), parseCWEvent/processLogEvents build one labels map and reuse it, by reference, for |
There was a problem hiding this comment.
parseCWEvent/processLogEvents build one labels map and reuse it, by reference
Should parseCWEvent/processLogEvents be making the map copy for entry{}s in the first place then instead of setting up a reference only for it to be copied downstream? The semantics are a bit cleaner that way IMO
Alternatively, is it possible/would it make sense to have processing fix up the labels map before hand and have a separate collection for structured metadata entries, and pass both of those collections by reference?
2 participants
Summary
Clone entry.labels to survive downstream mutation
Test plan
Test_batch_add_SharedLabelsAcrossBatch, which batches 3 entries sharing one labels map through astructured_metadatastage and asserts all 3 keep the field.go test ./pkg/...passes.