-
Notifications
You must be signed in to change notification settings - Fork 77
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: agent collector statistics report #3544
Conversation
continue | ||
} | ||
|
||
// TODO: Get ctx from request |
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.
Philosophical question: is there a way to get the context from the request?
My impression is that there is no way to get these contexts due to the passive nature of getting the messages.
Could we create a context in the future and change it by monitoring the stream connection?
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.
We would need to propagate headers through the messages to be able to create a context from the request (propagate the otel headers)
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.
GJ!
@@ -7,14 +7,14 @@ import ( | |||
"github.com/kubeshop/tracetest/agent/proto" | |||
) | |||
|
|||
func (c *Client) SendDataStoreConnectionResult(ctx context.Context, response *proto.DataStoreConnectionTestResponse) error { | |||
func (c *Client) SendOTLPConnectionResult(ctx context.Context, response *proto.OTLPConnectionTestResponse) error { |
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.
Is the renaming of this function correct? seems that there is a new file about oltp
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.
Not correct, nice catch. Unfortunately this endpoint isn't covered by a test, that's why I didn't get this error :(
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.
fixed the mess and also added tests to ensure it will not break in the future
This PR makes the agent report some statistics to the server when requested.
Checklist