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

tracing_subscriber: add additional fields for printing to json fmt subscriber #2943

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mladedav
Copy link
Contributor

@mladedav mladedav commented Apr 20, 2024

Motivation

Closes #1531

There are scenarios where one wants to add some information to logs emitted by FmtSubscriber, for example adding OpenTelemetry trace ID and span ID.

Solution

This adds an extension which all other subscribers can fill with any fields and then the formatting subscriber can emit these fields alongside the normal ones.

The extension currently wraps a HashMap<String, String>, but it could also feasibly be something like HashMap<String, Box<dyn Value>> but the flexibility of having other types than string permissible here didn't seem to offset the additional allocations for this String also needs an allocation. Otherwise we could use HashMap<String, serde_json::Value> if this would be considered to be json-specific but I don't think we should go that way now.

Currently, only the Json formatter prints these fields as other ones do not print span attributes at all.

@mladedav mladedav requested review from hawkw, davidbarsky and a team as code owners April 20, 2024 19:05
Copy link
Contributor

@kaffarell kaffarell left a comment

Choose a reason for hiding this comment

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

Gave this a quick spin and it seems to work great!
Maybe we should add an example though? The InjectingSubscriber setup is not so easy for someone who just wants to add some fields.

@mladedav
Copy link
Contributor Author

I've added an example with usage.

@mladedav
Copy link
Contributor Author

I tried to make this work with dyn Value but I can't figure out how to call Value::record so that I get the json representation. For that I need a Field and the only way I have found to get it is to construct Metadata which is hardly what should happen here.

@kaffarell
Copy link
Contributor

I tried to make this work with dyn Value but I can't figure out how to call Value::record so that I get the json representation. For that I need a Field and the only way I have found to get it is to construct Metadata which is hardly what should happen here.

What do you mean by this?

Copy link
Contributor

@kaffarell kaffarell left a comment

Choose a reason for hiding this comment

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

Example looks good!

@mladedav
Copy link
Contributor Author

Currently, the extension wraps HashMap<String, String> but that means among other things that numbers get represented as strings.

I tried changing that to HashMap<String, Box<Dyn Value>> but couldn't make it work.

When tracing starts to use valuable, it could probably use that instead but for now the string should hopefully be enough.

@kaffarell
Copy link
Contributor

Currently, the extension wraps HashMap<String, String> but that means among other things that numbers get represented as strings.

I tried changing that to HashMap<String, Box<Dyn Value>> but couldn't make it work.

When tracing starts to use valuable, it could probably use that instead but for now the string should hopefully be enough.

ooh, because it's not serializable, right.
Anyway strings are fine I guess.

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.

Allow configuring a formatter with extra fields to include
2 participants