-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
opentelemetry: add span event attributes #6531
opentelemetry: add span event attributes #6531
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
changelogs/fragments/6531-opentelemetry-add-event-attributes.yml
Outdated
Show resolved
Hide resolved
@v1v do you want to make more changes, or is this ready for merging? |
there are some vendors that might not require those attributes since those details are shown in the UI when accessing the spans, i.e.: jaeger
I think It's now ready to be reviewed, I was not happy with the initial implementation, so there is a new flag to disable this behaviour. Some OTEL vendors might visualise the span attributes in the logs while others won't, hence the feature flag to disable when no needed. Thanks for your review 🙏 |
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Felix Fontein <felix@fontein.de>
This looks great @v1v! Looking forward to having it in the next release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I glanced over the change, it looks good to me. If nobody objects I'll merge in a couple of days.
Backport to stable-7: 💚 backport PR created✅ Backport PR branch: Backported as #6631 🤖 @patchback |
* add span event attributes (task name and host name) * add fragment * refactor: use set_attributes * Add same span attributes to the event * chore: change description in the fragment * as mentioned in the code review * use flag to disable the attributes in logs there are some vendors that might not require those attributes since those details are shown in the UI when accessing the spans, i.e.: jaeger * Update plugins/callback/opentelemetry.py Co-authored-by: Felix Fontein <felix@fontein.de> --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit 58958fc)
@v1v thanks for your contribution! |
…attributes (#6631) opentelemetry: add span event attributes (#6531) * add span event attributes (task name and host name) * add fragment * refactor: use set_attributes * Add same span attributes to the event * chore: change description in the fragment * as mentioned in the code review * use flag to disable the attributes in logs there are some vendors that might not require those attributes since those details are shown in the UI when accessing the spans, i.e.: jaeger * Update plugins/callback/opentelemetry.py Co-authored-by: Felix Fontein <felix@fontein.de> --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit 58958fc) Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>
span.end(end_time=host_data.finish) | ||
# This will avoid populating span attributes to the logs | ||
span.add_event(task_data.dump, attributes={} if disable_attributes_in_logs else attributes) | ||
span.end(end_time=host_data.finish) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug, I've just found
SUMMARY
Enable populating span attributes to event attributes with OpenTelemetry in order to allow logs correlations with traces.
This is quite handy when the OTEL ventor does not show those details in the UI directly.
ISSUE TYPE
COMPONENT NAME
opentelemetry
ADDITIONAL INFORMATION
#4175 was the original implementation.
Closes #6526
Given
And if I run with the logs:
Produces
And if I run setting the attributes in the logs to false:
Produces