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

journald: remove span field prefixes; add separate fields for span data #1968

Merged
merged 11 commits into from
Apr 8, 2022

Conversation

wiktorsikora
Copy link
Contributor

@wiktorsikora wiktorsikora commented Mar 5, 2022

Motivation

Currently, tracing-journald prefixes event fields with the number
of parent spans, if present, in order to namespace field values. This
turned out to be unnecessary as journald preserves values for duplicated
field names.

Solution

This branch removes event field prefixes and emits parent span names
and their field/value pairs as additional key/value pairs.

@wiktorsikora wiktorsikora requested review from hawkw, davidbarsky and a team as code owners March 5, 2022 09:58
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

thanks for working on this! i noticed a few things we may want to address before this is ready to merge --- in particular, i think the relationship between the span field prefix and field prefix configurations seems a bit surprising.

also, it would be nice to hear from @Ralith before merging this.

tracing-journald/src/lib.rs Outdated Show resolved Hide resolved
tracing-journald/src/lib.rs Outdated Show resolved Hide resolved
tracing-journald/src/lib.rs Outdated Show resolved Hide resolved
tracing-journald/src/lib.rs Outdated Show resolved Hide resolved
tracing-journald/src/lib.rs Outdated Show resolved Hide resolved
@Ralith
Copy link
Collaborator

Ralith commented Mar 8, 2022

I didn't actually realize journald preserved values for duplicate field names. Maybe we should drop the span prefixes entirely? This would be breaking, but probably gives more intuitive/natural results in the common case.

We definitely want a custom prefix for span names and built-in metadata (SPAN_NAME, SPAN_CODE_LINE, etc. maybe?) but perhaps not for user-defined fields.

@wiktorsikora
Copy link
Contributor Author

Thanks for review. I have applied suggestions.

I agree it would be less confusing to disable prefixing of the user span fields by default.

Is there anything else I could do on this?

@Ralith
Copy link
Collaborator

Ralith commented Mar 10, 2022

I agree it would be less confusing to disable prefixing of the user span fields by default.

What about removing support for prefixing spans entirely? It seems superfluous to me in light of journald's behavior as tested here. I'm not sure anyone actually wants the current behavior, even as an option. I'd also like to get @hawkw's input on the compatibility break, too; if we go this way, it should probably be 0.2 so it doesn't catch anyone relying on the current behavior by surprise.

@wiktorsikora
Copy link
Contributor Author

wiktorsikora commented Mar 13, 2022

I removed span fields prefixing. I also added SPAN_NAME field emitted for events recorded inside spans.
As @Ralith pointed out, these are breaking changes.

tracing-journald/src/lib.rs Outdated Show resolved Hide resolved
tracing-journald/tests/journal.rs Outdated Show resolved Hide resolved
tracing-journald/src/lib.rs Outdated Show resolved Hide resolved
tracing-journald/tests/journal.rs Show resolved Hide resolved
tracing-journald/src/lib.rs Outdated Show resolved Hide resolved
tracing-journald/tests/journal.rs Outdated Show resolved Hide resolved
tracing-journald/tests/journal.rs Outdated Show resolved Hide resolved
tracing-journald/tests/journal.rs Outdated Show resolved Hide resolved
@wiktorsikora
Copy link
Contributor Author

@hawkw is there anything else I could do on this? No pressure, I just want to know :)

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

i think this looks good to me, thanks!

@psztoch
Copy link

psztoch commented Mar 30, 2022

When will these changes go to the master?
The proposed changes are VERY desirable.

@davidbarsky davidbarsky changed the title tracing-journald - option to disable prefix of user-defined span fields tracing-journald: remove span field prefixes; add separate fields for span data Apr 8, 2022
@davidbarsky davidbarsky changed the title tracing-journald: remove span field prefixes; add separate fields for span data journald: remove span field prefixes; add separate fields for span data Apr 8, 2022
@davidbarsky davidbarsky enabled auto-merge (squash) April 8, 2022 17:44
@hawkw hawkw disabled auto-merge April 8, 2022 17:57
@hawkw hawkw merged commit b158b77 into tokio-rs:master Apr 8, 2022
hawkw pushed a commit that referenced this pull request Apr 8, 2022
…ta (#1968)

## Motivation

Currently, `tracing-journald` prefixes event fields with the number
of parent spans, if present, in order to namespace field values. This
turned out to be unnecessary as journald preserves values for duplicated
field names.

## Solution

This branch removes event field prefixes and emits parent span names
and their field/value pairs as additional key/value pairs.
@hawkw hawkw mentioned this pull request Apr 8, 2022
hawkw pushed a commit that referenced this pull request Apr 9, 2022
…ta (#1968)

## Motivation

Currently, `tracing-journald` prefixes event fields with the number
of parent spans, if present, in order to namespace field values. This
turned out to be unnecessary as journald preserves values for duplicated
field names.

## Solution

This branch removes event field prefixes and emits parent span names
and their field/value pairs as additional key/value pairs.
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.

5 participants