out_opentelemetry: make add_label have an effect on logs#7029
out_opentelemetry: make add_label have an effect on logs#7029Frefreak wants to merge 4 commits intofluent:masterfrom
Conversation
Frefreak
requested review from
edsiper,
fujimotos,
koleini and
leonardo-albertovich
as code owners
Frefreak
force-pushed
the
add_labels_logs
branch
from
c92964f to
e89d4c5
Compare
Signed-off-by: carsonzhu <carsonzhu@tencent.com>
Frefreak
force-pushed
the
add_labels_logs
branch
from
e89d4c5 to
25b78bb
Compare
|
Is there anything i'm missing? The 2 workflows seem to be stuck on "Waiting for status to be reported". |
Signed-off-by: carsonzhu <carsonzhu@tencent.com>
Frefreak
force-pushed
the
add_labels_logs
branch
from
1c82ad4 to
1b62a1a
Compare
|
No, it's awaiting approval until we allow it as a new contributor - otherwise it opens the door to people submitting malicious PRs just to execute workloads or try to expose secrets. |
|
Docs are required to be updated as well, they currently make it clear labels only apply to metrics: Please provide a docs PR as well and link it to this PR. |
|
Thanks for the clarification. Docs added: fluent/fluent-bit-docs#1060 |
|
Gentle bump for a review. |
|
Can we please get this reviewed? I'm looking for a way to add Resource attribute so that I can add |
leonardo-albertovich
left a comment
leonardo-albertovich
left a comment
There was a problem hiding this comment.
Overall I'd prefer this to be implemented in a holistic manner with the log event attribute facility which would also entail adding support for it to processor_logs but considering the invested effort and timing this could be good enough to make the feature available.
| resource_logs[0] = &resource_log; | ||
|
|
||
| kv_size = mk_list_size(&(ctx->kv_labels)); | ||
| attributes_list = flb_calloc(kv_size, sizeof(Opentelemetry__Proto__Common__V1__KeyValue *)); |
There was a problem hiding this comment.
The code that depends on kv_size holding a value higher than zero should be conditional so it doesn't run unless necessary.
| } | ||
| attributes = flb_calloc(kv_size, sizeof(Opentelemetry__Proto__Common__V1__KeyValue)); | ||
| if (attributes == NULL) { | ||
| flb_errno(); |
There was a problem hiding this comment.
attributes_list is leaked in this code path.
| kv = mk_list_entry(kv_head, struct flb_kv, _head); | ||
| opentelemetry__proto__common__v1__key_value__init(&attributes[kv_index]); | ||
| attributes[kv_index].key = kv->key; | ||
| attributes[kv_index].value = otlp_any_value_initialize(MSGPACK_OBJECT_STR, 0); |
There was a problem hiding this comment.
We need to verify the result of otlp_any_value_initialize here and have a proper cleanup sequence.
There was a problem hiding this comment.
Maybe this loop could be merged with the one above.
| kv_index++; | ||
| } | ||
|
|
||
| resource_log.resource = flb_calloc(1, sizeof(Opentelemetry__Proto__Resource__V1__Resource)); |
There was a problem hiding this comment.
We need to check the result of flb_calloc here and have the appropriate cleanup sequence as well.
|
Thanks for the comments, will make the modifications later (probably tomorrow) |
Signed-off-by: Xiangyu Zhu <frefreak.zxy@gmail.com>
Frefreak
force-pushed
the
add_labels_logs
branch
from
660ab0d to
9c1dfe8
Compare
|
Hi all, could we please get his reviewed again? |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Hi guys. |
|
As far as I understand this needs to get reviewed in order to proceed (I had made the modifications with regarding to the comments). If there's anything else that needs to be changed I'm willing to modify accordingly. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
any news on this? |
|
There are a couple of recent OTel changes in Fluent Bit, specifically around Logs handling in the input and output, those changes will be available in v3.1 release, some references: |
|
Hi @edsiper, |
|
Never mind, processors can be specified for output Maybe it will be useful for someone: |
8 participants
This patch makes
add_labeloption in opentelemetry output plugin has an effect on logs data. Previously only metricsdata will get the labels added. This is useful because then logs can then be easily filtered using their labels (provided I set different labels config for different workloads)
The labels (kv) will be added to opentelemetry logs data, in
Resourceattributes (notLogRecordattributes) so multipleLogRecords can share the same key-value pairs.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Configuration
Debug/Valgrind log output
opentelemetry screenshot
below is opentelemetry collector's logging exporter's output, the labels can be seen in the "Resource" section
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.