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 guidance in README on how to set span attributes #123

Open
adam-fowler opened this issue May 11, 2023 · 4 comments
Open

Add guidance in README on how to set span attributes #123

adam-fowler opened this issue May 11, 2023 · 4 comments
Milestone

Comments

@adam-fowler
Copy link
Contributor

Accessing span attributes is most likely accessed behind a lock. To avoid locking and unlocking continuously it is preferable span attributes are built up separate from the span and then set once on the span.

Eg replace

span.attributes["attr1"] = value1
span.attributes["attr2"] = value2
...

with

var attributes = span.attributes
attributes["attr1"] = value1
attributes["attr2"] = value2
...
span.attributes = attributes
@adam-fowler
Copy link
Contributor Author

The alternative would be to replace the API such that you can't set single attributes.

@stevapple
Copy link
Contributor

stevapple commented May 11, 2023

var attributes = span.attributes
attributes["attr1"] = value1
attributes["attr2"] = value2
...
span.attributes = attributes

I suspect if this is the best practice, since it’s risk breaking data integrity by overwriting attributes set by other actors. For most users, setting one by one should be the easiest and safest. A possibility addition for bulk access to span attributes might be

span.withAttributes { attributes in
    attributes["attr1"] = value1
    attributes["attr2"] = value2
}

@ktoso
Copy link
Member

ktoso commented May 25, 2023

I like the bulk change idea, that's viable -- thank you @stevapple.

I think we can add these after the 1.0 which we're about to cut

@ktoso ktoso added this to the 1.1.x milestone May 25, 2023
@slashmo
Copy link
Collaborator

slashmo commented Sep 6, 2023

This change was added via #133, but we might want to highlight it in the documentation as well.

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

No branches or pull requests

5 participants