-
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: support stackdriver trace span (#4223) #4224
Conversation
leak check is here.
|
Ref: [fluent-bit #4224](fluent/fluent-bit#4224) Signed-off-by: 0Delta <[email protected]>
debug log is here.
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
@qingling128 can you review this one from usability perspective ? |
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
CI is failing due to a potential memory leak in the changes: https://github.com/fluent/fluent-bit/runs/4790455332?check_suite_focus=true#step:5:3982 |
I missed cleanup function. |
f800739
to
643b80c
Compare
Could you approve for run CI ? |
I can't see anything to approve here, do you want to give it a poke @0Delta - it may have been some error we've fixed or just some weird Github action issue. If you can push a new commit to refresh things it may spring to life. |
@patrick-stephens can you check pls why CI is stuck ? |
It looks like the problem we had a while back with missing skipped options but that was fixed a long time ago. I notice the fork is a bit behind so it might be worth rebasing to give it a poke @0Delta |
Ah, it appears to be an issue with Trying to figure out why as the error itself is incorrect. |
@0Delta can you please rebase your PR on top of master so the CI can pick the changes ? |
The int test ones use |
Apparently, the code that this PR is trying to change has been significantly changed by #5044. |
logentry will be able to bind to span in the Google Cloud Web UI. Specifically Cloud Trace (Stackdriver), the `logging.googleapis.com/traceId` field is moved to the top of the log entry and renames it to `traceId`. Signed-off-by: 0Delta <[email protected]>
I rebase and the test feels good. |
Ref: [fluent-bit #4224](fluent/fluent-bit#4224) Signed-off-by: 0Delta <[email protected]>
Could we revive this? Wanted to open this myself, came across this PR. |
This is now possible today. The fields were renamed since this PR was made, but |
Ref: [fluent-bit #4224](fluent/fluent-bit#4224) Signed-off-by: 0Delta <[email protected]> Signed-off-by: esmerel <[email protected]> Co-authored-by: esmerel <[email protected]>
logentry will be able to bind to span in the Google Cloud Web UI.
Specifically Cloud Trace (Stackdriver),
the
logging.googleapis.com/traceId
field ismoved to the top of the log entry and renames it to
traceId
.Signed-off-by: 0Delta [email protected]
#4223
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:
Documentation
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.