[beatreceiver] Preserve metadata from OTel log body for LS exporter#50145
Conversation
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
This comment has been minimized.
This comment has been minimized.
| // getEventMeta gives beat.Event.Meta from the context metadata | ||
| // The value of `[@metadata][beat]` is taken from the `Index` option of logstash output. | ||
| // In Elastic Agent, `Index` option is not available, hence, the value of `[@metadata][beat]` is derived from `IndexPrefix` | ||
| func getEventMeta(ctx context.Context) map[string]any { |
There was a problem hiding this comment.
We don't need to add this metadata field. They are already being added here by logstash client in beats. https://github.com/khushijain21/beats/blob/logstash-metadata/libbeat/outputs/codec/json/event.go#L46-L51
Can also be proven by e2e tests when these are removed
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Blocked by. https://github.com/elastic/ingest-dev/issues/7389. Waiting on how we decide to preserve metadata going out of otelconsumer - changes here will proceed accodingly |
Let's fix this for Logstash so that it is completely functional and deal with generalizing to Kafka later. Logstash is much more widely used than Kafka right now. |
|
The cost of rework here is something we can accept if we change the approach later, it's more important that the Logstash exporter works as it needs to. |
|
Looks like #50191 is ready to merge already |
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
x-pack/otel/exporter/logstashexporter/internal/event.go (1)
25-35:⚠️ Potential issue | 🟠 MajorPromote body
@metadataintobeat.Event.Metabefore serialization.
libbeat/outputs/codec/json/event.goemitsbeat.Event.Metaas@metadataand inlinesFields; leaving source@metadatainFieldscreates two writers for the same key. Also, deletingbeat/version/typefrom the top-level body can drop user fields. Extractfields["@metadata"]intoevent.Meta, delete it fromFields, and trim generated keys from that metadata map only.🐛 Proposed fix
- removeRedundantMetadataFields(fields) + metadata := extractEventMetadata(fields) timestamp, ok := parseEventTimestamp(fields) if !ok { timestamp = logRecord.ObservedTimestamp().AsTime() } event := beat.Event{ Timestamp: timestamp, Fields: fields, + Meta: metadata, }+func extractEventMetadata(fields map[string]any) map[string]any { + rawMetadata, ok := fields["@metadata"].(map[string]any) + if !ok { + return nil + } + + delete(fields, "@metadata") + removeRedundantMetadataFields(rawMetadata) + return rawMetadata +} + // removeRedundantMetadataFields removes certain metadata fields that will be generated again when the event is serialized. // See https://github.com/elastic/beats/blob/v9.3.3/libbeat/outputs/codec/json/event.go#L43-L54 // Not removing these fields would create duplicate fields and bloat the final event size -func removeRedundantMetadataFields(fields map[string]any) { - delete(fields, otelctx.MetadataBeatKey) - delete(fields, otelctx.MetadataVersionKey) - delete(fields, "type") +func removeRedundantMetadataFields(metadata map[string]any) { + delete(metadata, otelctx.MetadataBeatKey) + delete(metadata, otelctx.MetadataVersionKey) + delete(metadata, "type") }Also applies to: 62-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/otel/exporter/logstashexporter/internal/event.go` around lines 25 - 35, The code must extract any source metadata from fields["@metadata"] into the beat.Event.Meta before serialization to avoid duplicated `@metadata` keys; modify the construction around removeRedundantMetadataFields / parseEventTimestamp / beat.Event so that you look up v := fields["@metadata"], if present cast to map[string]interface{} and assign it to event.Meta, then delete fields["@metadata"] from Fields; ensure subsequent trimming/removal of generated keys (like "beat","version","type") is applied only to that extracted meta map rather than deleting top-level user fields in Fields; keep the existing timestamp fallback logic from parseEventTimestamp as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@x-pack/otel/exporter/logstashexporter/internal/event.go`:
- Around line 25-35: The code must extract any source metadata from
fields["@metadata"] into the beat.Event.Meta before serialization to avoid
duplicated `@metadata` keys; modify the construction around
removeRedundantMetadataFields / parseEventTimestamp / beat.Event so that you
look up v := fields["@metadata"], if present cast to map[string]interface{} and
assign it to event.Meta, then delete fields["@metadata"] from Fields; ensure
subsequent trimming/removal of generated keys (like "beat","version","type") is
applied only to that extracted meta map rather than deleting top-level user
fields in Fields; keep the existing timestamp fallback logic from
parseEventTimestamp as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9165e98c-6cac-4a58-97d7-31613e6b7a57
📒 Files selected for processing (5)
x-pack/otel/exporter/logstashexporter/exporter.gox-pack/otel/exporter/logstashexporter/exporter_test.gox-pack/otel/exporter/logstashexporter/internal/batch.gox-pack/otel/exporter/logstashexporter/internal/batch_test.gox-pack/otel/exporter/logstashexporter/internal/event.go
This comment has been minimized.
This comment has been minimized.
|
I don't see a test proving that |
This comment has been minimized.
This comment has been minimized.
I added a similar test i.e system/metics with logstash output in beats. But looks like input_id and raw_index metadata are only added when run from inside elastic-agent. So we can only confirm inside elastic-agent |
|
/test |
|
I tested this changes in elastic-agent here and it works |
This comment has been minimized.
This comment has been minimized.
TL;DRThree Linux FIPS jobs failed before tests ran due Remediation
Investigation detailsRoot Cause
Evidence
Verification
Follow-up
Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | noneWhat is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
Force merging, test failures are unrelated |
…50145) (#50274) * when metadata is passed via otelconsumer * fix tests * add docker-compose back * remove metadta * include metadata in e2e test * fix event size * address comments --------- (cherry picked from commit 15c13a5) Co-authored-by: Khushi Jain <khushi.jain@elastic.co> Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…lastic#50145) * when metadata is passed via otelconsumer * fix tests * add docker-compose back * remove metadta * include metadata in e2e test * fix event size * address comments --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…lastic#50145) * when metadata is passed via otelconsumer * fix tests * add docker-compose back * remove metadta * include metadata in e2e test * fix event size * address comments --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…lastic#50145) * when metadata is passed via otelconsumer * fix tests * add docker-compose back * remove metadta * include metadata in e2e test * fix event size * address comments --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…lastic#50145) * when metadata is passed via otelconsumer * fix tests * add docker-compose back * remove metadta * include metadata in e2e test * fix event size * address comments --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
Proposed commit message
This PR preserves metadata from OTel log body going out of LS exporter
Checklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.Disruptive User Impact
None
Related issues
Note:
A follow up PR on elastic-agent will include_metadata:true for logstash output elastic/elastic-agent#13713