-
Notifications
You must be signed in to change notification settings - Fork 1.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
fix: move analytics middleware #6901
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
2 similar comments
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Reviewed everything up to f9b4d70 in 1 minute and 52 seconds
More details
- Looked at
655
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/http/middleware/analytics.go:115
- Draft comment:
Re-closing and reopening the request body can lead to unexpected behavior if the body is read multiple times. Consider using a buffer to store the body content for reuse. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. pkg/http/middleware/analytics.go:1
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Gd9SEDpt9JJrDIHU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Incremental review on 78f430b in 47 seconds
More details
- Looked at
84
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/http/middleware/analytics.go:11
- Draft comment:
Theauth
package is marked for removal but is still being used in the code. Consider removing or replacing it as per the TODO comment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While the comment correctly identifies the TODO, it's not providing actionable feedback. The auth package is deeply integrated into the middleware's functionality for user authentication and telemetry. Simply removing it without a replacement strategy would break the code. The TODO comment is likely meant for a larger architectural change that needs proper planning.
The comment might be highlighting a legitimate architectural concern about auth package dependencies. The TODO could be part of a larger refactoring plan I'm not aware of.
Even if there is a plan to remove the auth package, this comment doesn't provide actionable guidance and the current usage appears intentional and necessary for the middleware's function.
Delete the comment as it's not actionable and the auth package is currently necessary for the middleware's core functionality.
2. pkg/http/middleware/analytics.go:35
- Draft comment:
Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. This is applicable to the use ofauth.AttachJwtToContext
andauth.GetEmailFromJwt
. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_4D6rzs1VNdYuNPfv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Incremental review on dc8cd05 in 32 seconds
More details
- Looked at
90
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. ee/query-service/app/server.go:325
- Draft comment:
The functionNewAnalytics
is used here, but it was previouslyNewAnalyticsMiddleware
. Ensure consistency in naming across the codebase for clarity. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions moving the analytics middleware to a common package without changing the logic. The changes in the code reflect this intent, as the middleware is now being used from a different package. However, there is a minor inconsistency in the naming of the middleware function, which should be addressed for clarity and consistency.
2. pkg/query-service/app/server.go:265
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
3. pkg/query-service/app/server.go:295
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_4IkyBgQOVguljpv5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Part of https://github.com/SigNoz/platform-pod/issues/406
this just moves the analystics middleware to common package , no change in logic. We will look into auth package that we are importing in this middleware later
Important
Moved analytics middleware to a common package without changing its logic.
ee/query-service/app/server.go
andpkg/query-service/app/server.go
topkg/http/middleware/analytics.go
.s.analyticsMiddleware
withmiddleware.NewAnalytics(zap.L()).Wrap
increatePrivateServer
andcreatePublicServer
functions inserver.go
files.auth
package import in the middleware will be reviewed later.This description was created by for dc8cd05. It will automatically update as commits are pushed.