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

When creating my own FormatEvent type, I have to duplicate a lot of code #1039

Open
cecton opened this issue Oct 15, 2020 · 13 comments
Open
Labels
crate/subscriber Related to the `tracing-subscriber` crate needs/design Additional design discussion is required.

Comments

@cecton
Copy link

cecton commented Oct 15, 2020

Feature Request

Crates

tracing-subscriber

Motivation

I'm trying to customize the log lines in the terminal. For that I need to write my own FormatEvent.

I copy-pasted the code of the one provided by tracing-subsriber called Format. But I had to duplicate a lot of other objects before being able to make it useful: FmtLevel, FmtThreadName, FmtCtx, even format::time::write()

Example here

Proposal

Not sure. Maybe make some common things pub?

Alternatives

Not sure either.

cc @davidbarsky

@hawkw
Copy link
Member

hawkw commented Oct 27, 2020

Yeah, the current FormatEvent interface could use some work. I'd like to prioritize improving this as part of the upcoming tracing-subscriber 0.3 release.

@hawkw hawkw added crate/subscriber Related to the `tracing-subscriber` crate needs/design Additional design discussion is required. labels Oct 27, 2020
@hawkw hawkw added this to the tracing-subscriber 0.3 milestone Oct 27, 2020
@dvdplm
Copy link
Contributor

dvdplm commented Oct 27, 2020

@hawkw let me know if you want any help with this. :)

@hawkw
Copy link
Member

hawkw commented Oct 28, 2020

@dvdplm thanks for the offer! Right now, I think we mostly need to answer some API design questions --- what do we need to add, and what do we need to change? It would be very helpful to get answers to the following:

  • What behaviors do users implementing FormatEvent want pre-built implementations of?
    • What "knobs" or points for additional customization do those pre-built implementations need to provide to be usable?
  • What's hard to use or restrictive about the current API? How can we change it to make it easier to use?

@cecton
Copy link
Author

cecton commented Nov 6, 2020

I'm putting this link here as an example of what I needed to duplicate: https://github.com/paritytech/substrate/blob/2e7292cd84121db8bcd2317c1ad70e348ee52f7a/client/cli/src/logging.rs

But there is going to be a lot more because now I have a use case where I want to make a Layer to replace fmt's Layer. This is because we also run the application in the browser so it should use console.log() instead of a Writer. There is a crate that does that already (tracing-wasm) but it ignores the FormatEvent. See the issue here for more details.

In the list of issues I have with the current API:

  1. too many things I need are not pub so I have to duplicate quite a lot of code to make my own FormatEvent
  2. I can't create my own "format Layer" because FmtContext is needed in input for FormatEvent but it has private fields and no constructor.

@cecton
Copy link
Author

cecton commented Nov 6, 2020

@hawkw maybe it's far fetched but would it be okay if I make a PR where I simply put some stuff public? That would reduce the size and complexity of my code already. It's not that I mind personally but we are a lot of people to maintain it and it's kinda complex. (Code is here)

@hawkw
Copy link
Member

hawkw commented Nov 6, 2020

I'm a little wary of making too much of the current interface pub --- then, it becomes a breaking change to remove it in the future. Rather than making FmtContext public and committing to maintaining it forever, I'd prefer to refactor the API so that the FmtContext type isn't necessary (#658).

@cecton
Copy link
Author

cecton commented Nov 6, 2020

Makes perfect sense... I'm not sure I will be able to help much on this refactoring. I think it's gonna be pretty deep. But don't hesitate to ping me.

In the source code I linked there are a lot of "TODO" marked with "duplicated code" or "inspired from" so you can see the issue I encountered. I think this file is the more representative of the "missing pubs".

On the other hand I also had issues substituting the "format Layer" (the layer that actually writes to the Writer). This is in this file. Probably what tracing needs is giving the user the ability to opt in (or not) of the default "format layer".

Lastly there is the problem of accessing the EventFormat object (also in that file). Ideally it should be accessible from any Layer so one can implement their own "format layer" and use the EventFormat that has been set.

Last lastly: to be honest --but not entirely related to this issue-- we also encountered an issue with the key-value thingy in the span macros. They allow a few types (u64, i64, String and bool) but in my use case I need to send a HashMap of some sort. In slog they had a more flexible key-value thingy that uses serde's Serialize so we could send properly (we were using slog before).

@dvdplm
Copy link
Contributor

dvdplm commented Nov 6, 2020

Last lastly: to be honest --but not entirely related to this issue-- we also encountered an issue with the key-value thingy in the span macros. They allow a few types (u64, i64, String and bool) but in my use case I need to send a HashMap of some sort. In slog they had a more flexible key-value thingy that uses serde's Serialize so we could send properly (we were using slog before).

@cecton Would #886 help here?

@cecton
Copy link
Author

cecton commented Nov 6, 2020

@dvdplm No... I saw it already but I don't think it will 😞 The thing I sent is missing a key in the JSON as you can see here. If we could send the whole thing un-serialized (like it was with slog, see here) I wouldn't need to serialize early like I did, I would serialize it in the Layer's on_event where I have the span's id. My current solution right now is hacky but fast: I serialize to JSON once and then I tweak the String to inject the span's id.

@hawkw
Copy link
Member

hawkw commented Nov 6, 2020

Last lastly: to be honest --but not entirely related to this issue-- we also encountered an issue with the key-value thingy in the span macros. They allow a few types (u64, i64, String and bool) but in my use case I need to send a HashMap of some sort. In slog they had a more flexible key-value thingy that uses serde's Serialize so we could send properly (we were using slog before).

@cecton Would #886 help here?

I think the issue that would help here is #819. We're definitely interested in adding serde support in the new value system for tracing 0.2.

@hawkw
Copy link
Member

hawkw commented Oct 21, 2021

Changes like #1651 may be helpful here as well.

@awused
Copy link

awused commented Jul 21, 2024

I wanted to make a relatively simple change (chop off my_crate:: from my own logs and shorten target strings) but the amount of copied code and extra dependencies - like needing to add nu-ansi-term (for replicating styling) and tracing-log (for NormalizedEvent) - is rather prohibitive. It seems like whatever is going to be done to make this better is going to be a breaking change either way, and it's been a few years, so maybe it's worth reconsidering if things like FmtLevel and Writer::dimmed/bold really need to stay private?

@thomaseizinger
Copy link
Contributor

I have a similar motivation. I wanted to explore with moving the printing of spans to the end of the log line instead of at the beginning. That would make logs easier to read because the actual message always starts at more or less the same indentation and additional context follows after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/subscriber Related to the `tracing-subscriber` crate needs/design Additional design discussion is required.
Projects
None yet
Development

No branches or pull requests

6 participants