out_stackdriver: print tag with api error message#8459
Merged
braydonk merged 1 commit intofluent:masterfrom Feb 16, 2024
Merged
out_stackdriver: print tag with api error message#8459braydonk merged 1 commit intofluent:masterfrom
braydonk merged 1 commit intofluent:masterfrom
Conversation
ryanohnemus
requested review from
braydonk,
igorpeshansky and
qingling128
as code owners
braydonk
reviewed
Feb 16, 2024
Contributor
braydonk
left a comment
braydonk
left a comment
There was a problem hiding this comment.
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
force-pushed
the
stackdriver_tag_on_error
branch
from
e3a8c0a to
1d95059
Compare
Contributor
Author
|
@braydonk great suggestion! just pushed the change |
braydonk
reviewed
Feb 16, 2024
| 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, |
Contributor
There was a problem hiding this comment.
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.
ryanohnemus
force-pushed
the
stackdriver_tag_on_error
branch
from
1d95059 to
15b5e4a
Compare
Signed-off-by: ryanohnemus <ryanohnemus@gmail.com>
ryanohnemus
force-pushed
the
stackdriver_tag_on_error
branch
from
15b5e4a to
40ab261
Compare
braydonk
approved these changes
Feb 16, 2024
Contributor
|
Thanks for the PR! |
Contributor
Author
|
@braydonk those test failures look like flakes (time limit issues?) is there anything i need to do for this to be merged? |
Contributor
|
Probably a flake, I'll re-run it just in case |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-testlabel 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.