Skip to content
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

feat: support syslog appender #72

Merged
merged 12 commits into from
Nov 12, 2024
Merged

feat: support syslog appender #72

merged 12 commits into from
Nov 12, 2024

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Nov 12, 2024

This closes fast/fasyslog#3.

@tisonkun
Copy link
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 ...

@tisonkun tisonkun enabled auto-merge (squash) November 12, 2024 02:55
@andylokandy
Copy link
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
9 checks passed
@tisonkun tisonkun deleted the syslog branch November 12, 2024 02:58
/// 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
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
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
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 🤣

Copy link
Contributor

@andylokandy andylokandy Nov 12, 2024

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...

Copy link
Contributor

@andylokandy andylokandy Nov 12, 2024

Choose a reason for hiding this comment

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

ping 😆

Copy link
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
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
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 >_<

@@ -29,7 +29,7 @@ fn test_meta_logging_in_format_works() {
.max_file_size(1024 * 1024)
.build("logs")
.unwrap();
let (writer, _guard) = NonBlockingBuilder::default().finish(rolling);
let (writer, _guard) = rolling_file::non_blocking_builder().finish(rolling);
Copy link
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
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
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