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

Add level metadata to spans #182

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Add level metadata to spans #182

merged 2 commits into from
Nov 7, 2024

Conversation

mTsBucy1
Copy link
Contributor

See #180 for a description.

This PR implements the OpenTelemetryLayer::with_level setting, to disable (enabled by default) attaching the level tag to exported spans.

There are several concerns I see before recommending merging this:

  • Will enable by default break code which manually set the level field? If so, disable-by-default seems smarter.
  • Event levels are still always exported, should we also restrict this?
  • I am somewhat confused, but it seems error level events set the span status to error. Is this behaviour intuitive, once we have a level attached to the span? Should error spans always set the span status?

@djc
Copy link
Collaborator

djc commented Nov 5, 2024

@mladedav any opinions on this? I don't have a strong opinion.

@mladedav
Copy link
Contributor

mladedav commented Nov 5, 2024

Looks good to me and I think this will be useful.

This should just also include a changelog entry for anyone who sets some kind of level explicitly as a span field.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

@mladedav thanks for the quick followup!

src/layer.rs Outdated Show resolved Hide resolved
@djc djc merged commit 95f9ab9 into tokio-rs:v0.1.x Nov 7, 2024
14 checks passed
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.

3 participants