app/vlinsert/opentelemetry: fix field collisions after flattening#1379
app/vlinsert/opentelemetry: fix field collisions after flattening#1379yagmurcicekdagi wants to merge 1 commit intoVictoriaMetrics:masterfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Hi, Can you review this pr ? @func25? I've fixed the field name collisions, haven't implemented the query/merge path duplicate name issue yet. Let me know what you think so I can adjust the code. |
Signed-off-by: Yağmur Çiçekdağı <ygmcicekdagi@gmail.com>
yagmurcicekdagi
force-pushed
the
fix_otel_field_collision
branch
from
d6d6440 to
b8779ba
Compare
|
Hey, thank you for your interest in this issue.
I'd recommend starting with a few smaller issues first to get familiar with the logic, the code conventions and the contribution guidelines. As I skimmed through it, this PR appears to be a low-quality AI-generated contribution. |
|
I did use AI to help me locate the issue and give me skeleton to work on, but if you mean the abstractions that I put , that was from me unfortunately -which breaks the contribution guidelines Could you maybe guide me a bit to how to make this code better quality? I'd still love to work on getting this to better quality code, but if you don't want that, that's totally understandable as well Thanks for the reply @func25 |
func25
left a comment
func25
left a comment
There was a problem hiding this comment.
No problem. I saw some improvements/bug fixes in this PR itself, it's not entirely AI-generated slop, so haven't closed it. Please see below comments.
Using AI to enhance quality and make small improvements is encouraged, but not for this kind of task (yet).
| const ( | ||
| resourcePrefix = "resource" | ||
| attributesPrefix = "attributes" | ||
| bodyPrefix = "body" | ||
| ) | ||
|
|
||
| // fieldPrefixes holds the field name prefixes for each OpenTelemetry source. | ||
| // Empty string means no prefix (default behaviour). | ||
| type fieldPrefixes struct { | ||
| resource string | ||
| attributes string | ||
| body string | ||
| } | ||
|
|
||
| func newFieldPrefixes(enabled bool) fieldPrefixes { | ||
| if !enabled { | ||
| return fieldPrefixes{} | ||
| } | ||
| return fieldPrefixes{ | ||
| resource: resourcePrefix, | ||
| attributes: attributesPrefix, | ||
| body: bodyPrefix, | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's avoid this mini-framework and put the code directly into decoder functions, e.g. decodeResource, decodeLogRecord, ...
| } | ||
|
|
||
| func decodeResourceLogs(src []byte, pushLogs pushLogsHandler) (err error) { | ||
| func decodeResourceLogs(src []byte, pushLogs pushLogsHandler, prefixes fieldPrefixes) (err error) { |
There was a problem hiding this comment.
| func decodeResourceLogs(src []byte, pushLogs pushLogsHandler, prefixes fieldPrefixes) (err error) { | |
| func decodeResourceLogs(src []byte, pushLogs pushLogsHandler, useFieldNamePrefixes bool) (err error) { |
| effectiveMsgFields := msgFields | ||
| if useFieldPrefixes && len(msgFields) == 0 { | ||
| // When field prefixes are enabled, scalar body values are stored under the "body" field name. | ||
| // Auto-rename "body" -> "_msg" so the body still appears as the log message by default. | ||
| effectiveMsgFields = []string{"body"} | ||
| } |
There was a problem hiding this comment.
It's better to avoid this kind of nuance here
| return "", 0, fmt.Errorf("cannot read Body") | ||
| } | ||
| if err := decodeAnyValue(body, fs, fb, ""); err != nil { | ||
| if err := decodeAnyValue(body, fs, fb, prefixes.body); err != nil { |
There was a problem hiding this comment.
| if err := decodeAnyValue(body, fs, fb, prefixes.body); err != nil { | |
| if err := decodeAnyValue(body, fs, fb, "", bodyKeyValueListFieldNamePrefix); err != nil { |
2 participants
Changes
Fixes: #1371
-opentelemetry.enableFieldPrefixesflag (disabled by default). When enabled, fields are prefixedby their source before flattening:
Added tests covering prefix mode for all field sources and verify generated fields
(trace_id, span_id, severity_number, severity_text) remain unprefixed.
Updated docs/victorialogs/data-ingestion/opentelemetry.md with a Field prefixes
section and victoria_logs_common_flags.md with the new flag.