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

Fix issue with invalid breadcrumb metadata #974

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

tombruijn
Copy link
Member

We had a report of breadcrumbs not working when the metadata was invalid. This required some manual intervention from us to fix the data. Avoid this occurring in the future by not allowing any other metadata than a Hash object.

Closes https://github.com/appsignal/support/issues/269

We had a report of breadcrumbs not working when the metadata was
invalid. This required some manual intervention from us to fix the data.
Avoid this occurring in the future by not allowing any other metadata
than a Hash object.

Closes appsignal/support#269
Copy link
Contributor

@unflxw unflxw left a comment

Choose a reason for hiding this comment

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

Looks good as-is, some thoughts:

Would it be useful/convenient for users if add_breadcrumb tried to call .to_h on the value? (and then verified that that is a Hash)

Also, going by the docs, the keys and values should both be strings. We could check for that as well.

@tombruijn
Copy link
Member Author

Would it be useful/convenient for users if add_breadcrumb tried to call .to_h on the value? (and then verified that that is a Hash)

I don't think that's necessary. We can require people to call to_h themselves. We don't do this anywhere else.

Also, going by the docs, the keys and values should both be strings. We could check for that as well.

Strings, Symbols, integers, any flat object seems to be supported. I'll add some extra checks.

@tombruijn
Copy link
Member Author

Looks like the internal structure doesn't really matter, as long as the metadata object itself is a Hash. I'll leave the PR as-is.

image

@tombruijn tombruijn merged commit 9cdee8a into main Jun 27, 2023
@tombruijn tombruijn deleted the check-breadcrumb-metadata-argument branch July 31, 2023 09:30
@tombruijn tombruijn restored the check-breadcrumb-metadata-argument branch July 31, 2023 09:30
@tombruijn tombruijn deleted the check-breadcrumb-metadata-argument branch July 10, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants