Skip to content

feat: support syslog appender - #72

Merged
tisonkun merged 12 commits into
mainfrom
syslog
Nov 12, 2024
Merged

tisonkun merged 12 commits into
mainfrom
syslog

Conversation

@tisonkun

@tisonkun tisonkun commented Nov 12, 2024

Copy link
Copy Markdown
Contributor

This closes fast/fasyslog#3.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun

Copy link
Copy Markdown
Contributor Author

cc @andylokandy you may be interested at 272ea1e.

I'm considering add a feature flag to bundle re-export otel in fastrace anyway ...

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
tisonkun enabled auto-merge (squash) November 12, 2024 02:55
@andylokandy

Copy link
Copy Markdown
Contributor

I'm considering add a feature flag to bundle re-export otel in fastrace anyway ...

how will the reexported otel interact, where one is from logforth and the other is from fastrace?

@tisonkun
tisonkun merged commit 7ab3a8c into main Nov 12, 2024
@tisonkun
tisonkun deleted the syslog branch November 12, 2024 02:58
Comment thread src/append/syslog.rs
/// Set the layout of the [`Syslog`] appender.
///
/// Default to `None`, only the args will be logged.
pub fn with_layout(mut self, layout: impl Into<Layout>) -> Self {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The builder style is not inline with OpentelemetryLogBuilder:

let builder = OpentelemetryLogBuilder::new("my_service", "http://localhost:4317");
builder.label("env", "production");

Maybe choose one and apply for all of them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andylokandy You may see 41640f7 and 0c8dce7.

I ever think of using one pattern, but later found that layout is good for builder, while other appenders are not builders, so layout can be misleading with a getter method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe anyway they are builder-alike so that we treat all of them builders 🤣

@andylokandy andylokandy Nov 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'll prefer with_layout. A single word .layout sounds like a verb, while it actually has no side effect...

@andylokandy andylokandy Nov 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping 😆

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that we should change methods of OpentelemetryLogBuilder to align with the with_ flavor?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Others should stay with the current with_ flavor and nothing need to be changed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with the current method, you can send a patch with expected changes and let's discuss there >_<

.build("logs")
.unwrap();
let (writer, _guard) = NonBlockingBuilder::default().finish(rolling);
let (writer, _guard) = rolling_file::non_blocking_builder().finish(rolling);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rolling_file::non_blocking().finish(rolling) or even rolling_file::non_blocking(rolling).finish()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rolling_file::non_blocking(rolling).finish() looks good. Let me give it a try.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #73

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrate with log crate in logforth

2 participants