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

fix: if tracing is enabled, use if for logging too #48

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Apr 24, 2024

When having crates which use tracing and log and have the tracing feature enabled, you'll get mixed log formats on the output:

2024-04-24T11:56:23.290168Z  INFO sea_orm_migration::migrator: Applying migration 'm0000220_create_package_relates_to_package'
2024-04-24T11:56:23.291295Z  INFO sea_orm_migration::migrator: Migration 'm0000220_create_package_relates_to_package' has been applied
2024-04-24T11:56:23.291484Z  INFO sea_orm_migration::migrator: Applying migration 'm0000230_create_qualified_package_transitive_function'
2024-04-24T11:56:23.291994Z  INFO sea_orm_migration::migrator: Migration 'm0000230_create_qualified_package_transitive_function' has been applied
2024-04-24T11:56:23.292199Z  INFO sea_orm_migration::migrator: Applying migration 'm0000240_create_importer'
2024-04-24T11:56:23.294018Z  INFO sea_orm_migration::migrator: Migration 'm0000240_create_importer' has been applied
[WARN  trustify_module_graph::graph::sbom::spdx] Replacing faulty SPDX license expression with NOASSERTION: Parsing for expression `Parsing for expression `GPLV2+,-LGPLV2+,-MIT` failed.` failed.
[WARN  trustify_module_graph::graph::sbom::spdx] Replacing faulty SPDX license expression with NOASSERTION: Parsing for expression `Parsing for expression `GPLV2+,-LGPLV2+,-MIT` failed.` failed.
[WARN  trustify_module_graph::graph::sbom::spdx] Replacing faulty SPDX license expression with NOASSERTION: Parsing for expression `Parsing for expression `GPLV2+,-LGPLV2+,-MIT` failed.` failed.
[WARN  trustify_module_graph::graph::sbom] unable to ingest relationships involving a non-fully-qualified package pkg://golang/github.com/openshift/ocs-operator
[WARN  trustify_module_graph::graph::sbom] unable to ingest relationships involving a non-fully-qualified package pkg://generic/gnulib
[WARN  trustify_module_graph::graph::sbom] unable to ingest relationships involving a non-fully-qualified package pkg://pypi/:sys-platform
[INFO  trustify_module_graph::graph::sbom::spdx] Package cache: PackageCache { cache: 8044, hits: 24040 }

This was due to the fact that we enabled the trace feature, but did not disable the default (and thus the log) feature. Having both enabled, will also enable both frameworks, one for logging, one for tracing.

Disabling the log feature however removes the log output.

I think the correct fix would be to enable the tracing-subscriber/tracing-log feature, which captures both log and tracing events, streaming both the tracing system, giving a consolidated format.

This would make the log and trace feature mutually exclusive.

@ctron
Copy link
Contributor Author

ctron commented Apr 24, 2024

Additionally, this enables the ansi feature of tracing-subscriber, to have colored output (unless the NO_COLOR env-var is set:

image

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 28, 2024

This would make the log and trace feature mutually exclusive.

That's a firm no-no. Features should be additive.

Perhaps if everything is transparently funneled through tracing if both features are enabled and otherwise the logic stays the same this would be workable.

@ctron ctron force-pushed the feature/align_log_1 branch 2 times, most recently from 3a2351e to f746a54 Compare April 29, 2024 06:44
@ctron
Copy link
Contributor Author

ctron commented Apr 29, 2024

Ok, so I reworked it in a way that:

  • when you enable log, you get env_logger.
  • when you enable tracing, you get tracing-subscriber
  • when you enable both log and tracing, you get both (logs and traces) through tracing-subscriber

Copy link
Owner

@d-e-s-o d-e-s-o left a comment

Choose a reason for hiding this comment

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

Core attribute logic this seems fine to me now, thanks! Please revert spurious formatting changes, though. Also, can you please create a separate pull request for the ansi feature addition? This will require enabling of color for env_logger as well, so that both are colored by default. Right now we get colored logs when both features are enabled and uncolored when only log is active.

@ctron
Copy link
Contributor Author

ctron commented Apr 29, 2024

Please revert spurious formatting changes, though.

That was applied by rustfmt, I was assuming that project was using it too. I can try to revert those.

@ctron
Copy link
Contributor Author

ctron commented Apr 29, 2024

Ok, this PR should now only contain the trace-logging stuff.

@d-e-s-o d-e-s-o merged commit eb9a182 into d-e-s-o:main Apr 29, 2024
6 checks passed
@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 29, 2024

Thank you!

d-e-s-o added a commit that referenced this pull request Apr 29, 2024
Add a CHANGELOG entry for #48, which enabled the tracing-subscriber's
tracing-log feature to unify log output if both the 'log' and 'trace'
feature are enabled.
@ctron ctron deleted the feature/align_log_1 branch April 29, 2024 15:17
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.

None yet

2 participants