-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Be explicit about span vs resource attributes #282
Conversation
776ab9e
to
68e2f40
Compare
@@ -13,15 +13,15 @@ def configure_tracer( | |||
enabled: bool, | |||
endpoint: str, | |||
service_name: str, | |||
service_attributes: dict[str, Any] | None = None, | |||
resource_attributes: dict[str, Any] | None = None, |
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 will break the api, so we need to do a major release for this change. I like that the api will be consistent with the visualization in grafana.
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 will probably also use the opportunity to make other minor breaking api changes before we 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.
Yes
Can we add span attribute
as a argument somewhere as well.
Hiding the ddtrace set tags config?
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.
Experience has showed that wrapping the ddtrace api is not popular. So I would just let it be :)
Breaking change.
Be explicit about span vs resource attributes and update docs.
https://grafana.com/docs/tempo/latest/operations/best-practices/#span-and-resource-attributes
https://opentelemetry.io/docs/specs/otel/overview/#resources