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(faro/receiver): propagate request metadata to downstream consumers #6515

Merged
merged 7 commits into from
Apr 4, 2024

Conversation

hainenber
Copy link
Contributor

@hainenber hainenber commented Feb 25, 2024

PR Description

Which issue(s) this PR fixes

Fixes #6503

Notes to the Reviewer

Added a E2E test for the whole flow but afraid that it's plagued with data race 😞 . Hope this can be circumvented by running the test without the race detector.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@hainenber hainenber marked this pull request as ready for review February 27, 2024 17:26
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

Doc change is small. Over to Agent team for code review.

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Feb 27, 2024
@clayton-cornell clayton-cornell requested a review from a team February 27, 2024 17:43
@ptodev ptodev self-assigned this Mar 19, 2024
Copy link
Contributor

@ptodev ptodev left a comment

Choose a reason for hiding this comment

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

Thank you, and apologies for the late review! If it's hard to get the test working, maybe a simpler test will do for now?

docs/sources/flow/reference/components/faro.receiver.md Outdated Show resolved Hide resolved
@clayton-cornell
Copy link
Contributor

This one is still sitting in my ToDos...
@hainenber there are some conflicts here that need to be resolved.
@ptodev is it ready for merge from your side once the conflicts are sorted?

@hainenber hainenber force-pushed the include-metadata-for-faro-receiver branch from de7b30d to dbc9ffa Compare March 23, 2024 16:07
@hainenber
Copy link
Contributor Author

I've resolved the merge conflict. Re: simpler test for the change, most of metadata handling logic comes from internals of OpenTelemetry collectors so a E2E test is pretty much warranted. I think we can skip running data race for this particular test for the time being.

@rfratto
Copy link
Member

rfratto commented Apr 4, 2024

@hainenber I'm really sorry about how long this (and your other PRs) have been sitting around; we've all been super busy preparing for GrafanaCon. Working on helping getting this merged now :)

@rfratto rfratto merged commit 2e95228 into grafana:main Apr 4, 2024
10 checks passed
@hainenber hainenber deleted the include-metadata-for-faro-receiver branch April 5, 2024 14:40
@hainenber
Copy link
Contributor Author

@rfratto no worries, the more GA users from GrafanaCon, the better for me also :D

Thanks for checking out my past PRs!

@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label May 6, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an include_metadata argument to faro.receiver
4 participants