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-journald: make level mappings configurable #2347

Conversation

little-dude
Copy link

Motivation

My company uses different conventions to map tracing levels to journald priorities. I need to be able to configure the mappings to integrate with existing services.

Solution

Do not hardcode the mappings. Instead store the mappins to use in the subscriber.

@little-dude little-dude requested review from hawkw, davidbarsky and a team as code owners October 12, 2022 20:09
@little-dude little-dude force-pushed the tracing-journald-priority-mappings branch from bd18969 to 7673d7c Compare October 12, 2022 20:19
@little-dude little-dude force-pushed the tracing-journald-priority-mappings branch 2 times, most recently from 217e857 to ca8c393 Compare October 14, 2022 19:01
@little-dude little-dude force-pushed the tracing-journald-priority-mappings branch 2 times, most recently from 6eb08e5 to 0d2b814 Compare October 19, 2022 15:31
@hawkw
Copy link
Member

hawkw commented Oct 19, 2022

CI docs builds seem to be failing across the board, so that's unrelated...

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.

some very minor documentation suggestions, but other than that, this looks good to me! thank you!

@little-dude little-dude force-pushed the tracing-journald-priority-mappings branch from 8470211 to b86aabc Compare October 21, 2022 08:39
@little-dude
Copy link
Author

also, an example would be nice, but not a big deal.

I added an example and a test.

@little-dude little-dude force-pushed the tracing-journald-priority-mappings branch from 7ab29e7 to b28463a Compare October 21, 2022 12:39
@little-dude little-dude force-pushed the tracing-journald-priority-mappings branch from b28463a to 78e545e Compare October 26, 2022 12:37
@little-dude little-dude requested review from hawkw and Ralith and removed request for davidbarsky, hawkw and Ralith November 8, 2022 06:15
@t-lintanf
Copy link

Hello,
What is the status on this PR ? I am also in the need to re-map the level mapping.

@little-dude
Copy link
Author

little-dude commented Feb 14, 2023

It needs to be reviewed because I added a test and an example since the first review.

@little-dude
Copy link
Author

@t-lintanf also note that this MR is for master currently. If you need this in 0.1, it'll need to be backported.

@hawkw
Copy link
Member

hawkw commented Feb 21, 2023

ah, sorry i didn't see the most recent changes, my bad!

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.

one last suggestion: i think it makes more sense to put the example on the with_priority_mappings function's RustDoc, rather than in examples/. once that's addressed, I think this will be good to merge!

Comment on lines +14 to +24
registry
.with(
subscriber
// We can tweak the mappings between the trace level and
// the journal priorities.
.with_priority_mappings(PriorityMappings {
info: Priority::Informational,
..PriorityMappings::new()
}),
)
.init();
Copy link
Member

Choose a reason for hiding this comment

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

I think this example ought to be in the RustDoc documentation for the with_priority_mappings method, rather than in the general example file.

@kaffarell
Copy link
Contributor

Hey @little-dude do you think we could get this merged? Otherwise I could pick it up as well...

@kaffarell
Copy link
Contributor

@hawkw, @davidbarsky looks like this PR is stale, applied the changes to current master and opened a new PR ( #2824 ).
I think we can close this one and continue the discussion on the new PR...

@hawkw hawkw closed this in d4ca988 Jan 11, 2024
hds pushed a commit that referenced this pull request Nov 19, 2024
This allows to manually map tracing levels to journald levels.

It seems that @little-dude, who started the original PR, doesn't have
time to finish this, so I picked it up.  Reapplied the changes to the
newest master branch and fixed the latest comments/issues.

This will also fix/close:

Closes #2649
Closes #2661
Closes #2347 (the original pr)
hds pushed a commit that referenced this pull request Nov 20, 2024
This allows to manually map tracing levels to journald levels.

It seems that @little-dude, who started the original PR, doesn't have
time to finish this, so I picked it up.  Reapplied the changes to the
newest master branch and fixed the latest comments/issues.

This will also fix/close:

Closes #2649
Closes #2661
Closes #2347 (the original pr)
hds pushed a commit that referenced this pull request Nov 20, 2024
This allows to manually map tracing levels to journald levels.

It seems that @little-dude, who started the original PR, doesn't have
time to finish this, so I picked it up.  Reapplied the changes to the
newest master branch and fixed the latest comments/issues.

This will also fix/close:

Closes #2649
Closes #2661
Closes #2347 (the original pr)
hds pushed a commit that referenced this pull request Nov 20, 2024
This allows to manually map tracing levels to journald levels.

It seems that @little-dude, who started the original PR, doesn't have
time to finish this, so I picked it up.  Reapplied the changes to the
newest master branch and fixed the latest comments/issues.

This will also fix/close:

Closes #2649
Closes #2661
Closes #2347 (the original pr)
hds pushed a commit that referenced this pull request Nov 21, 2024
This allows to manually map tracing levels to journald levels.

It seems that @little-dude, who started the original PR, doesn't have
time to finish this, so I picked it up.  Reapplied the changes to the
newest master branch and fixed the latest comments/issues.

This will also fix/close:

Closes #2649
Closes #2661
Closes #2347 (the original pr)
hds pushed a commit that referenced this pull request Nov 22, 2024
This allows to manually map tracing levels to journald levels.

It seems that @little-dude, who started the original PR, doesn't have
time to finish this, so I picked it up.  Reapplied the changes to the
newest master branch and fixed the latest comments/issues.

This will also fix/close:

Closes #2649
Closes #2661
Closes #2347 (the original 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.

5 participants