-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[extensions/observer/cfgardenobserver] Implement cfgardenobserver #34513
[extensions/observer/cfgardenobserver] Implement cfgardenobserver #34513
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just nits.
I haven't gotten a chance to look through tests yet, I wanted to get a review out sooner rather than later. Hope it's helpful 👍
I appreciate that, think I have addressed all the issues. Regarding the context in the struct, I'd like to know if you think what I did (just keeping a channel) is ok, or if I might be missing some need for a context. |
…en-telemetry#34513) **Description:** First Component PR: open-telemetry#33727 This is the second PR for adding the cfgardenobserver, with the first suggested implementation. There are definitely some decisions made that require feedback, such as adding the CloudFoundry application labels to the Endpoint labels, and the decision to use the `Container` EndpointType at all. **Link to tracking Issue:** open-telemetry#33618 **Testing:** Unit testing of config and extension **Documentation:** Updated readme with new configuration and endpoints --------- Co-authored-by: sam clulow <[email protected]> Co-authored-by: sam clulow <[email protected]> Co-authored-by: José Riguera Lopez <[email protected]>
Description:
First Component PR: #33727
This is the second PR for adding the cfgardenobserver, with the first suggested implementation. There are definitely some decisions made that require feedback, such as adding the CloudFoundry application labels to the Endpoint labels, and the decision to use the
Container
EndpointType at all.Link to tracking Issue: #33618
Testing: Unit testing of config and extension
Documentation: Updated readme with new configuration and endpoints