out_stackdriver: add project_id_key override to allow specifying gcp project id from the record #8209

ryanohnemus · 2023-11-22T16:25:55Z

A new configuration option for the out_stackdriver plugin project_id_key allows you to specify a key within the jsonPayload that will be used to set the GCP Project ID to export to. This allows a stackdriver out plugin that has logWriter permissions in multiple projects to write to different gcp projects based on a jsonPayload (record) key. This also introduces a default logging.googleapis.com/projectId key that will be used if present in the record. The key that is used from the record is removed similar to how severity_key and label_key work.

Addresses #8195

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

Example configuration file for the change

[FILTER]
    Name modify
    Match k8s_*project1*
    Add logging_project_id prj-fluent-bit-test1

[FILTER]
    Name modify
    Match k8s_*project2*
    Add logging_project_id prj-fluent-bit-test2

[OUTPUT]
    Name stackdriver
    Match k8s_*
    export_to_project_key logging_project_id
    resource k8s_container
    k8s_cluster_name my-test-cluster
    k8s_cluster_location us-central1

Debug log output from testing the change

Attached Valgrind output that shows no leaks or memory corruption was found

==97479== 
==97479== HEAP SUMMARY:
==97479==     in use at exit: 0 bytes in 0 blocks
==97479==   total heap usage: 995 allocs, 995 frees, 238,168 bytes allocated
==97479== 
==97479== All heap blocks were freed -- no leaks are possible
==97479== 
==97479== For lists of detected and suppressed errors, rerun with: -s
==97479== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

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
Docs PR: stackdriver: amend special fields docs fluent-bit-docs#1267

Configuration Parameters

Key	Description	default
project_id_key	The value of this field is used by the Stackdriver output plugin to find the gcp project id from jsonPayload and then extract the value of it to set the `PROJECT_ID` within LogEntry `logName`, which controls the gcp project that should receive these logs.	logging.googleapis.com/projectId, if not present within the jsonPayload the value of export_to_project_id is used as the gcp project id.

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.

braydonk · 2023-11-22T17:56:45Z

Thank you for the PR! I am sorry that I didn't see the issue opened last week, thank you for opening a PR about it.

This seems like a good change that we'll probably accept, however I think it would be best for it to match the implementation of the other special keys. Generally with other special keys that this plugin recognizes, we have a default in the format logging.googleapis.com/<keyName>, where the logging.googleapis.com allows it to be obvious in the log structure what fields are intended for the plugin to handle in a special way.

You can see for example where severity key is defined in the header:

fluent-bit/plugins/out_stackdriver/stackdriver.h

Line 52 in 8733b25

#define DEFAULT_SEVERITY_KEY "logging.googleapis.com/severity"

Then we have config options for alternatives to those keys to be specified:

fluent-bit/plugins/out_stackdriver/stackdriver.c

Line 2987 in 8733b25

FLB_CONFIG_MAP_STR, "severity_key", DEFAULT_SEVERITY_KEY,

What you have is most of the way to what is needed. The changes would essentially be:

Add a new constant in the header, DEFAULT_PROJECT_ID_KEY, and I would recommend making the value logging.googleapis.com/projectId
Change export_to_project_key in struct flb_stackdriver and the config to project_id_key. The new meaning of this config would be to override the default project ID key. The default value of this should be the constant. When the config is read, the value of ctx->project_id_key will always be set, either to the default or to the override from the config.
Add a test similar to the one you already added (thank you for doing that btw) that also tests logging.googleapis.com/projectId has the same behaviour as overriding the key.

Once those changes are made, I'm happy to accept the change!

(P.S. for that DCO check; all commits in this repo are required to be signed, so that's git commit -s. To correct the current one you can run git commit --amend -s --allow-empty and force push)

braydonk · 2023-11-22T17:58:34Z

plugins/out_stackdriver/stackdriver.c

@@ -2410,8 +2415,19 @@ static flb_sds_t stackdriver_format(struct flb_stackdriver *ctx,
        }

        /* logName */
-        len = snprintf(path, sizeof(path) - 1,
+        project_key_extracted = FLB_FALSE;


I think the extraction here should be moved up with the rest of the extractions, like line 2212ish.

The /* logName */ comment should also be moved down to where the log name check is now.

ryanohnemus · 2023-11-22T20:05:20Z

@braydonk I appreciate the quick review and the feedback! I just force pushed the latest changes... I hope this is in line with your comments. If I missed anything please let me know and I'll give it another shot!

braydonk

LGTM! One last thing

braydonk · 2023-11-22T20:24:21Z

plugins/out_stackdriver/stackdriver.c

@@ -2410,8 +2422,13 @@ static flb_sds_t stackdriver_format(struct flb_stackdriver *ctx,
        }

        /* logName */


This comment needs to be moved down to where the logName section is (line 2432)

missed that one... Just pushed the latest commit!

Testing/Temporary/CTestCostData.txt

braydonk

Thank you for the useful feature and for working with me in the review!

ryanohnemus · 2023-11-22T20:36:54Z

Thank you for guiding me through it @braydonk! I really appreciate the help!

Closes fluent#8195 Signed-off-by: ryanohnemus <[email protected]>

ryanohnemus · 2023-11-22T21:26:25Z

@braydonk I had to push another commit to fix the memory leak due to not cleaning up the extract project_id_key, so this will probably need another review/approval to kick off the workflow tests again.

I ran valgrind ./bin/flb-rt-out_stackdriver and with the following output:

SUCCESS: All unit tests have passed.
==62935== 
==62935== HEAP SUMMARY:
==62935==     in use at exit: 0 bytes in 0 blocks
==62935==   total heap usage: 201,986 allocs, 201,986 frees, 96,592,089 bytes allocated
==62935== 
==62935== All heap blocks were freed -- no leaks are possible
==62935== 
==62935== For lists of detected and suppressed errors, rerun with: -s
==62935== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

braydonk

Looks like the action failed is a flake.

Thank you for the PR!

ryanohnemus · 2023-12-05T17:34:39Z

I'm happy to have been able to contribute! As far as merging goes, does that happen in a standard cadence after approvals? (Not sure if I need to do anything else for this PR)

Do I wait until this is fully merged to create a documentation pull request? (Or will someone else handle the documentation update after merge?)

braydonk · 2023-12-05T17:59:12Z

I've got the Doc PR ready (added it into the description of this PR).

I was intending to merge this here now, but it turns out there's a file here we aren't covered as a CODEOWNER where we should be (the testdata file). I'm going to file a separate PR to get us added to that so I can subsequently merge this one.

braydonk · 2023-12-05T18:09:06Z

Opened the CODEOWNERS PR here: #8254
Once it's merged, I can merge this.

ryanohnemus · 2023-12-21T23:45:55Z

@braydonk not sure if you're still around because of the holidays, but wanted to check in and see if this could be merged now that the code owners pr was merged. It looks like there will be a release cut today from @edsiper's comments in #8289

Closes fluent#8195 Signed-off-by: ryanohnemus <[email protected]>

Closes fluent#8195 Signed-off-by: ryanohnemus <[email protected]> Signed-off-by: ahspw <[email protected]>

ryanohnemus requested review from braydonk, igorpeshansky, qingling128, edsiper, leonardo-albertovich, fujimotos and koleini as code owners November 22, 2023 16:25

github-actions bot added the docs-required label Nov 22, 2023

braydonk reviewed Nov 22, 2023

View reviewed changes

ryanohnemus force-pushed the feature/stackdriver_project_key branch 2 times, most recently from 58da0a3 to 0eed9ec Compare November 22, 2023 20:01

braydonk reviewed Nov 22, 2023

View reviewed changes

ryanohnemus temporarily deployed to pr November 22, 2023 20:25 — with GitHub Actions Inactive

ryanohnemus force-pushed the feature/stackdriver_project_key branch from 0eed9ec to 670d4a1 Compare November 22, 2023 20:27

ryanohnemus temporarily deployed to pr November 22, 2023 20:32 — with GitHub Actions Inactive

braydonk reviewed Nov 22, 2023

View reviewed changes

Testing/Temporary/CTestCostData.txt Outdated Show resolved Hide resolved

ryanohnemus force-pushed the feature/stackdriver_project_key branch from 670d4a1 to dec9f21 Compare November 22, 2023 20:35

braydonk previously approved these changes Nov 22, 2023

View reviewed changes

ryanohnemus temporarily deployed to pr November 22, 2023 20:36 — with GitHub Actions Inactive

ryanohnemus temporarily deployed to pr November 22, 2023 20:58 — with GitHub Actions Inactive

out_stackdriver: add project_id_key override

60919e5

Closes fluent#8195 Signed-off-by: ryanohnemus <[email protected]>

ryanohnemus dismissed braydonk’s stale review via 60919e5 November 22, 2023 21:23

ryanohnemus force-pushed the feature/stackdriver_project_key branch from dec9f21 to 60919e5 Compare November 22, 2023 21:23

ryanohnemus requested a review from braydonk November 28, 2023 17:34

ryanohnemus changed the title ~~out_stackdriver: add export_to_project_key override to allow specifying gcp project id from the record~~ out_stackdriver: add project_id_key override to allow specifying gcp project id from the record Dec 1, 2023

ryanohnemus temporarily deployed to pr December 4, 2023 16:27 — with GitHub Actions Inactive

ryanohnemus temporarily deployed to pr December 4, 2023 16:47 — with GitHub Actions Inactive

braydonk approved these changes Dec 5, 2023

View reviewed changes

braydonk approved these changes Jan 10, 2024

View reviewed changes

braydonk merged commit 697a413 into fluent:master Jan 10, 2024
45 of 47 checks passed

braydonk mentioned this pull request Jan 10, 2024

stackdriver: amend special fields docs fluent/fluent-bit-docs#1267

Merged

BrewTestBot mentioned this pull request Jan 15, 2024

fluent-bit 2.2.2 Homebrew/homebrew-core#159951

Merged

shaerpour pushed a commit to shaerpour/fluent-bit that referenced this pull request Jan 16, 2024

out_stackdriver: add project_id_key override (fluent#8209)

da1e006

Closes fluent#8195 Signed-off-by: ryanohnemus <[email protected]>

shaerpour pushed a commit to shaerpour/fluent-bit that referenced this pull request Jan 16, 2024

out_stackdriver: add project_id_key override (fluent#8209)

5c5c059

Closes fluent#8195 Signed-off-by: ryanohnemus <[email protected]> Signed-off-by: ahspw <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

out_stackdriver: add project_id_key override to allow specifying gcp project id from the record #8209

out_stackdriver: add project_id_key override to allow specifying gcp project id from the record #8209

ryanohnemus commented Nov 22, 2023 •

edited by braydonk

Loading

braydonk commented Nov 22, 2023

braydonk Nov 22, 2023

ryanohnemus commented Nov 22, 2023

braydonk left a comment

braydonk Nov 22, 2023

ryanohnemus Nov 22, 2023

braydonk left a comment

ryanohnemus commented Nov 22, 2023

ryanohnemus commented Nov 22, 2023

braydonk left a comment

ryanohnemus commented Dec 5, 2023

braydonk commented Dec 5, 2023

braydonk commented Dec 5, 2023

ryanohnemus commented Dec 21, 2023

		@@ -2410,8 +2422,13 @@ static flb_sds_t stackdriver_format(struct flb_stackdriver *ctx,
		}

		/* logName */

out_stackdriver: add project_id_key override to allow specifying gcp project id from the record #8209

out_stackdriver: add project_id_key override to allow specifying gcp project id from the record #8209

Conversation

ryanohnemus commented Nov 22, 2023 • edited by braydonk Loading

braydonk commented Nov 22, 2023

braydonk Nov 22, 2023

Choose a reason for hiding this comment

ryanohnemus commented Nov 22, 2023

braydonk left a comment

Choose a reason for hiding this comment

braydonk Nov 22, 2023

Choose a reason for hiding this comment

ryanohnemus Nov 22, 2023

Choose a reason for hiding this comment

braydonk left a comment

Choose a reason for hiding this comment

ryanohnemus commented Nov 22, 2023

ryanohnemus commented Nov 22, 2023

braydonk left a comment

Choose a reason for hiding this comment

ryanohnemus commented Dec 5, 2023

braydonk commented Dec 5, 2023

braydonk commented Dec 5, 2023

ryanohnemus commented Dec 21, 2023

ryanohnemus commented Nov 22, 2023 •

edited by braydonk

Loading