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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from Apr 8, 2022
Merged

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

merged 11 commits into from Apr 8, 2022

Conversation

wiktorsikora
Copy link
Contributor

@wiktorsikora wiktorsikora commented Mar 5, 2022 •
edited by davidbarsky

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
hawkw
hawkw reviewed Mar 8, 2022
View reviewed changes
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 •
edited

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 and others added 2 commits March 9, 2022 14:26
@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 •
edited

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.

Ralith
Ralith requested changes Mar 13, 2022
View reviewed 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
hawkw
hawkw reviewed Mar 14, 2022
View reviewed changes
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 :)

hawkw
hawkw approved these changes Mar 22, 2022
View reviewed changes
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
@wiktorsikora @hawkw
journald: remove span field prefixes; add separate fields for span da…
4eed50a 
…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
Merged
hawkw pushed a commit that referenced this pull request Apr 9, 2022
@wiktorsikora @hawkw
journald: remove span field prefixes; add separate fields for span da…
ce613f8 
…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.
@lights0123 lights0123 mentioned this pull request Apr 26, 2022
Merged
@davidbarsky davidbarsky mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ralith Ralith Ralith approved these changes

@davidbarsky davidbarsky davidbarsky approved these changes

@hawkw hawkw hawkw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wiktorsikora @Ralith @psztoch @davidbarsky @hawkw