-
Notifications
You must be signed in to change notification settings - Fork 44
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
Implement journald exporter for OpenTelemetry #84
base: main
Are you sure you want to change the base?
Conversation
@TommyCpp had an question on comparing it with the subscriber provided by Tokio tracing library - https://github.com/tokio-rs/tracing/blob/master/tracing-journald/ -
|
comment the json_format option.
This was motivated by the improved downstream developer experience from being pure Rust, and the simplicity of the journald client protocol. It's not significantly more complicated than FFIing to libsystemd, and it dramatically reduces the sensitivity of downstream crates to build environment issues like missing pkg-config files, linking during cross-compilation, etc. |
Thanks, @Ralith, for the background. The concerns regarding build complexities with FFI to C are valid, and we will consider documenting this for the developers. My motivation for using libsystemd is that it internally handles sending large data through memfd, and so abstracting these low-level details. And hopefully being future-proof with any protocol changes :) |
self | ||
} | ||
|
||
pub fn attribute_prefix(mut self, attribute_prefix: Option<String>) -> Self { |
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.
It would be more user friendly to just ask them for a string slice instead of having them to call to_string()
and wrapping it with an Option
.
pub fn attribute_prefix(mut self, attribute_prefix: Option<String>) -> Self { | |
pub fn attribute_prefix(mut self, attribute_prefix: &str) -> Self { |
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.
Done
} | ||
|
||
pub fn message_size_limit(mut self, message_size_limit: usize) -> Self { | ||
self.message_size_limit = Some(message_size_limit); |
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.
Add some validation? Something like message_size_limit > 0
?
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.
Done
} | ||
#[cfg(not(feature = "json"))] | ||
{ | ||
return Err(std::io::Error::new( |
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.
Move this validation to json_format
method where the user configures this bool variable instead of checking it in export method?
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.
Done
let identifier = self.identifier.ok_or("Identifier is required")?; | ||
let message_size_limit = self | ||
.message_size_limit | ||
.ok_or("Message size limit is required")?; |
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.
Could we have a default value for message_size_limit
instead of requiring the user to always specify this?
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.
Yes, default is 0 now, which means no limit is enforced.
self | ||
} | ||
|
||
pub fn build(self) -> Result<JournaldLogExporter, &'static str> { |
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.
Could we simply return JournadLogExporter
instead of a Result
?
pub fn build(self) -> Result<JournaldLogExporter, &'static str> { | |
pub fn build(self) -> JournaldLogExporter { |
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.
Done
|
||
fn init_logger() -> LoggerProvider { | ||
let exporter = JournaldLogExporter::builder() | ||
.identifier("opentelemetry-journal-exporter") |
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.
Assign with
prefix to the builder methods?
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.
Done
.identifier("opentelemetry-journal-exporter") | ||
.message_size_limit(4 * 1024) | ||
.attribute_prefix(Some("OTEL_".to_string())) | ||
.json_format(true) //uncomment to log in json format |
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.
The comment is unclear.
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.
Good catch, fixed now.
Changes
This adds
Journald
log exporter for OpenTelemetry, allowing logs to be sent to Journald. Note that this exporter is experimental, and breaking changes are expected. The performance and stability improvements are required, and contributions are welcome.Features:
memfd
can be used for sending large messages.Dependencies:
This implementation relies on the
libsystemd
library for sending log entries to the journald daemon. It usessd_journal_sendv
API method provided by the library.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes