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

feat: Add method to get event fields inside a Hook #682

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

erezrokah
Copy link

@erezrokah erezrokah commented Jul 22, 2024

Hi 👋 Thank you for this great module 🚀

Opening for feedback as an attempt to fix #493, #587, also referenced from https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5918/files#diff-a9b82897838da6b8eed398e03c9590621e6c2336ff576b3a49fb684ff8bba2a1R110

This is not the cleanest approach, but could be better from using reflection. It doesn't expose the internal buffer representation format so if it ever changes GetFields can be modified.

Feedback very much welcomed

@erezrokah erezrokah changed the title feat: Add method to get event fields feat: Add method to get event fields inside a Hook Jul 22, 2024
event.go Outdated Show resolved Hide resolved
event.go Outdated Show resolved Hide resolved
event.go Outdated Show resolved Hide resolved
@erezrokah
Copy link
Author

Thanks for the quick review @ccoVeille, followed up with the suggestions and added more tests. Also please see the comment about the tradeoff of using JSON Decode

Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

Thanks for applying the changes, here are a few more feedbacks

event.go Show resolved Hide resolved
event.go Show resolved Hide resolved
event.go Outdated Show resolved Hide resolved
erezrokah and others added 3 commits July 22, 2024 22:23
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
@erezrokah erezrokah requested a review from ccoVeille July 22, 2024 19:32
event_test.go Outdated Show resolved Hide resolved
Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

LGTM 🤞

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.

[question] How to access with fields inside a hook
2 participants