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

out_stackdriver: print tag with api error message #8459

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

ryanohnemus
Copy link
Contributor

When there is an api error (ie a permission denied, or other error) when submitting to the google logging api, we get a print out of the payload text, but that is not specific enough to debug the issue. Adding the chunk tag (which usually comes from an input plugin like tail) may assist in finding the root cause of the issue.

Ideally I'd like the output success / failure metrics to include the logName as a tag, but this is much more involved as this would require looping through every entry in the logging request to get the entry number and logName and that would have to be compared to the response. Maybe this is less work that i initially assessed but it would possibly be a good idea and something that we can put into a feature request in the future.


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

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.

Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

LGTM in general! One thing I usually see though with these types of error messages is that the typical pattern is to add tag=%s, so instead of error for %s: would you be willing to change it to tag=%s on each log?

@ryanohnemus
Copy link
Contributor Author

@braydonk great suggestion! just pushed the change

@@ -2942,15 +2942,18 @@ static void cb_stackdriver_flush(struct flb_event_chunk *event_chunk,
#endif
if (c->resp.status >= 400 && c->resp.status < 500) {
ret_code = FLB_ERROR;
flb_plg_warn(ctx->ins, "error: %s", c->resp.payload);
flb_plg_warn(ctx->ins, "error for tag=%s: %s", event_chunk->tag,
Copy link
Contributor

Choose a reason for hiding this comment

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

That's good, slight adjustment to the ordering:

Suggested change
flb_plg_warn(ctx->ins, "error for tag=%s: %s", event_chunk->tag,
flb_plg_warn(ctx->ins, "tag=%s error sending to Cloud Logging: %s", event_chunk->tag,

This applied to each of the messages. The convention is for the tag=%s to usually go right after the plugin/log level designations.

@braydonk
Copy link
Contributor

Thanks for the PR!

@ryanohnemus
Copy link
Contributor Author

@braydonk those test failures look like flakes (time limit issues?) is there anything i need to do for this to be merged?

@braydonk
Copy link
Contributor

Probably a flake, I'll re-run it just in case

@braydonk braydonk merged commit 7bcefbb into fluent:master Feb 16, 2024
43 checks passed
@ryanohnemus ryanohnemus deleted the stackdriver_tag_on_error branch February 16, 2024 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants