-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
There was a problem hiding this 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?
e3a8c0a
to
1d95059
Compare
@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, |
There was a problem hiding this comment.
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:
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.
1d95059
to
15b5e4a
Compare
Signed-off-by: ryanohnemus <[email protected]>
15b5e4a
to
40ab261
Compare
Thanks for the PR! |
@braydonk those test failures look like flakes (time limit issues?) is there anything i need to do for this to be merged? |
Probably a flake, I'll re-run it just in case |
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:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-test
label to test for all targets (requires maintainer to do).Documentation
Backporting
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.