Skip to content

opentelemetry: disable default tracing-log feature #1869

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

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

jtescher
Copy link
Collaborator

No description provided.

@jtescher jtescher added the crate/opentelemetry Related to the `tracing-opentelemetry` crate. label Jan 27, 2022
@jtescher jtescher requested a review from a team as a code owner January 27, 2022 17:52
@davidbarsky davidbarsky merged commit 2f10453 into master Feb 1, 2022
@davidbarsky davidbarsky deleted the jtescher/remove-tracing-log-default branch February 1, 2022 21:31
@hawkw
Copy link
Member

hawkw commented Feb 1, 2022

I'm not sure if this was the correct solution. All this is going to do, AFAICT, is break events generated from log records by default. The tracing-log dependency is not the tracing crate's log feature, which i think is what users were concerned about.

Can we take a moment to make sure this actually solves the problem before releasing it?

@davidbarsky
Copy link
Member

davidbarsky commented Feb 1, 2022

Can we take a moment to make sure this actually solves the problem before releasing it?

Yes. I'm sorry about that.

@hawkw
Copy link
Member

hawkw commented Feb 1, 2022

No worries, I just saw this merge and I hadn't been sure it was the right thing, so I wanted to make sure we had a chance to talk through it with @jtescher so that we're all on the same page here.

@jtescher
Copy link
Collaborator Author

jtescher commented Feb 1, 2022

Ah commented I in the less relevant thread perhaps 😅 #1877 (comment)

@jtescher
Copy link
Collaborator Author

jtescher commented Feb 1, 2022

@hawkw happy to revert this as well if you think it's best to leave the feature on by default.

@hawkw
Copy link
Member

hawkw commented Feb 1, 2022

I think the feature should be left on by default. All it does is ensure that any events generated by the log crate are correctly translated into tracing events with the correct field names and so on, it doesn't actually cause logs to be emitted. It can still be disabled by disabling tracing-opentelemetry's default features, so I'd prefer to not suddenly break people's events, personally; especially if this doesn't actually solve a specific reported issue...

@jtescher
Copy link
Collaborator Author

jtescher commented Feb 1, 2022

#1882

hawkw pushed a commit that referenced this pull request Feb 2, 2022
hawkw pushed a commit that referenced this pull request Feb 3, 2022
hawkw pushed a commit that referenced this pull request Feb 3, 2022
hawkw pushed a commit that referenced this pull request Feb 3, 2022
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/opentelemetry Related to the `tracing-opentelemetry` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants