Skip to content

feat: support attaching trace id to log #88

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

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Conversation

andylokandy
Copy link
Contributor

No description provided.

@andylokandy andylokandy requested a review from tisonkun January 8, 2025 06:40

use crate::Marker;

// TODO(tisonkun): use trait alias when it's stable - https://github.com/rust-lang/rust/issues/41517
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps not my ID here XD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to align with the same comment in CustomLayout

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Good to go. But I may need one more round to consider whether Marker a good abstraction. It looks added random kvs with very niche use case .. I can't map it to other common log concepts.

very niche use case

That said, we add bridges for it everywhere but use it under a very specific way.

#[cfg(feature = "fastrace")]
mod fastrace;

/// Represents a layout for formatting log records.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it's not a layout

Copy link
Contributor Author

@andylokandy andylokandy Jan 8, 2025

Choose a reason for hiding this comment

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

good catch!

@andylokandy
Copy link
Contributor Author

It's mainly to extract context where the log is emitted. User can attach tokio's task_id, thread_id or cpu_id.

@@ -55,6 +56,7 @@ pub struct OpentelemetryLogBuilder {
protocol: Protocol,
labels: Vec<(Cow<'static, str>, Cow<'static, str>)>,
layout: Option<Layout>,
marker: Option<Marker>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why Marker is an Option. It looks like arbitrary kvs to be attached, then it can be 0 to n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because user may not provide a marker implementation. If not using Option, we have to provide a NoopMarker.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, it may not be 0 or 1, but 0 to n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It can provide 0 to n extra kvs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. I get it now .. The marker is the context (driver) and the f passed in is the method to populate kvs..

@tisonkun
Copy link
Contributor

tisonkun commented Jan 8, 2025

This looks like some concepts like MDC - https://logback.qos.ch/manual/mdc.html

@andylokandy
Copy link
Contributor Author

I think it's similar concept. But MDC is hardly a better name IMO...

@tisonkun
Copy link
Contributor

tisonkun commented Jan 8, 2025

I think it's similar concept. But MDC is hardly a better name IMO...

Agreed. I get it now the Marker is a context extractor to attach arbitrary key values to the current log record. Let me consider a bit how we organize the API. Especially f: impl FnMut(&str, String) without type or comment is very confusing that why here is a function, what's the meaning of each args.

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

I get the point and each concept's reason now.

Let's merge it first while hold the release for a few days to consider possibly better name and code structure.

@tisonkun tisonkun merged commit 1882b79 into fast:main Jan 8, 2025
9 checks passed
let root = Span::root("root", SpanContext::random());
let _g = root.set_local_parent();

log::error!("Hello syslog error!");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops. need to correct in the next pr.

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.

2 participants