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

Expose BacktraceClient.SetAttribute #233

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hymerman
Copy link
Contributor

This is useful to set single attributes without discarding other existing ones, as SetAttributes does. It's useful to set attributes before the BeforeSend callback is called, since then they'll get saved out to be sent with native crashes, where this callback won't get called.

This is useful to set single attributes without discarding other existing
ones, as SetAttributes does. It's useful to set attributes before the
BeforeSend callback is called, since then they'll get saved out to be sent
with native crashes, where this callback won't get called.
@konraddysput
Copy link
Collaborator

The backtraceClient has [] override that allows to set a single attribute. Maybe it's not the clear from our API perspective, so exposing this to set a single attribute makes a lot of sense - however, if you look at the [] operator logic, there are more things that are happening (for example: setting an attribute also on the native layer). Can we move the code from the [] operator to your method and call your new method in the [] operator?

@hymerman
Copy link
Contributor Author

Thanks Konrad - you're absolutely right, the indexer can be called directly, and now I'm no longer sure why I added SetAttribute! I'm happy to either close this, or as you say, move the code around - which would you prefer?

@konraddysput
Copy link
Collaborator

I think it's fine if we can make it more obvious - I'm happy to add your changes if you change the [] operator implementation and adjust your business logic

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.

2 participants