-
Notifications
You must be signed in to change notification settings - Fork 20
Improve Beholder Logger #1280
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
base: main
Are you sure you want to change the base?
Improve Beholder Logger #1280
Conversation
bolekk
commented
Jun 19, 2025
- Don't drop errors
- Protect against blocking calls
1. Don't drop errors 2. Protect against blocking calls
7b379a2
to
eb095e7
Compare
@@ -146,3 +253,8 @@ func getLabelMap(keysAndValues ...any) map[string]string { | |||
} | |||
return labels | |||
} | |||
|
|||
func getBeholderCallContext() (context.Context, context.CancelFunc) { |
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.
i thought the entire point of chip was to guarantee delivery of beholder logs.
i understand the technical reason for a timeout.
but, i am concerned this is shortcutting the product requirements and expectations.
what am i missing?
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.
Not logs - events. Logs will be delivered on a best-effort basis.
(there is a small subtlety here that the emitter is currently packing logs into an event and I can't remember if that is going to change or if that event will be treated differently, maybe @patrickhuie19 can clarify)
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.
IIUC otel treats logs and events as one kind of thing on the same stream
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.
Based on what I understood from @hendoxc and Mathew, Beholder Client will select a different endpoint to send the data to, under the hood.
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.
All logs use the BaseMessage
proto format and go over the custom message endpoint.
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.
So is it OK to merge? :)