Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loki_out: add stuctured_metadata_map_keys #9530

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0x006EA1E5
Copy link

@0x006EA1E5 0x006EA1E5 commented Oct 25, 2024

Resolves #9463

  • Adds stuctured_metadata_map_keys config to dynamically populate stuctured_metadata from a map

Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

fluent/fluent-bit-docs#1527

Backporting

  • Backport to latest stable release.

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.

@0x006EA1E5 0x006EA1E5 force-pushed the loki_out-structured_metadata_map branch from 7f1db76 to a9cdcad Compare October 26, 2024 12:24
@patrick-stephens patrick-stephens added the ok-package-test Run PR packaging tests label Oct 28, 2024
@0x006EA1E5 0x006EA1E5 force-pushed the loki_out-structured_metadata_map branch from a9cdcad to e9fee7d Compare October 29, 2024 11:05
Copy link
Contributor

@patrick-stephens patrick-stephens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but @niedbalski what do you think?

@@ -0,0 +1,41 @@
services:
fluentbit:
image: fluent/fluent-bit:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably add a build section as well and comment out one or the other just to show we can either compile or use the latest.

Copy link
Author

@0x006EA1E5 0x006EA1E5 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this?:

    build:
      context: ../../
      dockerfile: dockerfiles/Dockerfile
    pull_policy: build

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for pull_policy and I'd just leave it commented out once you've tested it - main thing is it allows people to see how to build vs pull

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be working locally now

networks:
- loki-network
volumes:
- ./config/fluent-bit_loki_out-structured_metadata_map.yaml:/etc/fluent-bit_loki_out-structured_metadata_map.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mount this to the default location then you can also remove the command override - although that only is true currently if it is the legacy TOML format config.

e.g. this would mean no need to have a command:

      - ./config/fluent-bit_loki_out-structured_metadata_map.conf:/fluent-bit/etc/fluent-bit.conf

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this preferred? I find the yaml easier to read.

I think I read somewhere that you're going to make the yaml config the default? Would this end up diverging between the 3.x branch and 4.x?

Happy to change this though if that's what you want 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is fine and yeah we will be moving to YAML by default for 4.0.

pack_label_key(mp_pck, (char*) accessed_map_kv.key.via.str.ptr,
accessed_map_kv.key.via.str.size);
/*
* Does this need optimising? For example, to handle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple is better I think unless it causes an impact. I'd be tempted to keep a naive initial implementation that is simpler and optimise later with specific test cases then.

{"structured_metadata_map_and_explicit",
flb_test_structured_metadata_map_and_explicit},
{"structured_metadata_map_single_missing_map",
flb_test_structured_metadata_map_single_missing_map},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably but we will know when we find them!

@patrick-stephens
Copy link
Contributor

FYI I'm seeing the K8S event test fail in other PRs so I think it is a flake: https://github.com/fluent/fluent-bit/actions/runs/12834843167/job/35870345004?pr=9530

@patrick-stephens
Copy link
Contributor

@0x006EA1E5 could you clean up the commits and/or squash them?

@0x006EA1E5 0x006EA1E5 force-pushed the loki_out-structured_metadata_map branch 2 times, most recently from 560e444 to 8387b9a Compare January 23, 2025 14:38
#
# If you would like to disable reporting, uncomment the following lines:
#analytics:
# reporting_enabled: false
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I disable this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not unless required I'd say

* Adds stuctured_metadata_map_keys config to dynamically populate stuctured_metadata from a map
* Add docker-compose to test loki backend

Signed-off-by: Greg Eales <0x006EA1E5@gmail.com>
if (kv->ra_key != NULL && kv->ra_val == NULL) {

/* try to get the value for the record accessor */
if (flb_ra_get_kv_pair(kv->ra_key, *map, &start_key, &out_key, &out_val)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flb_ra_* API do not use msgpack-c return codes like MSGPACK_UNPACK_CONTINUE, need to check the return values and adjust it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-package-test Run PR packaging tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loki Output structured_metadata from Map-like data structure
4 participants