fix: use common timeout middleware #6866
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 562a2be in 37 seconds
More details
- Looked at
142lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/constants/constants.go:257
- Draft comment:
Consider using amap[string]boolforTimeoutExcludedRoutesfor consistency with other boolean maps in the codebase. - Reason this comment was not posted:
Confidence changes required:50%
The PR replaces the old timeout middleware with a new one from a common package. The old middleware functions are removed, and the new middleware is applied to both public and private servers. The change is consistent across the files, and the new middleware is correctly imported and used. The PR description aligns with the changes made.
2. pkg/query-service/constants/constants.go:258
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This comment applies to other similar instances in the code as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_KxbYuRZkk6m9NvNy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
grandwizard28
requested changes
Jan 21, 2025
|
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.
👍 Looks good to me! Incremental review on 22f823e in 47 seconds
More details
- Looked at
460lines of code in12files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. pkg/apiserver/apiserver.go:11
- Draft comment:
Consider returning a slice instead of a map forGetTimeoutExcludedRoutesas it represents a list of routes, and a slice would be more appropriate for this use case. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new interfaceAPIServerwith methods to get context timeout configurations. However, theGetTimeoutExcludedRoutesmethod returns a map, which is not ideal for a list of routes. A slice would be more appropriate.
2. pkg/apiserver/config.go:38
- Draft comment:
TheValidatemethod currently does nothing. Consider implementing validation logic for the configuration values to ensure they are within expected ranges or formats. - Reason this comment was not posted:
Confidence changes required:50%
TheNewConfigFactoryfunction inpkg/apiserver/config.gocorrectly initializes a new config factory. However, theValidatemethod currently does nothing. It might be beneficial to implement validation logic for the configuration values.
3. pkg/apiserver/provider.go:18
- Draft comment:
Consider adding validation for theconfigparameter in theNewfunction to ensure it contains valid values before using it to initialize the provider. - Reason this comment was not posted:
Confidence changes required:50%
TheNewfunction inpkg/apiserver/provider.goinitializes a provider with the given config. However, it does not perform any validation on the config. It might be beneficial to validate the config before using it.
4. pkg/query-service/app/server.go:267
- Draft comment:
The use ofmiddleware.NewTimeoutwith parameters froms.serverOptions.SigNoz.APIServeris consistent with the PR's intent to use common timeout middleware. Ensure that theAPIServerinterface and its implementations provide the correct values. - Reason this comment was not posted:
Confidence changes required:30%
ThecreatePrivateServerandcreatePublicServermethods inpkg/query-service/app/server.gouse themiddleware.NewTimeoutfunction with parameters froms.serverOptions.SigNoz.APIServer. This is consistent with the PR's intent to use common timeout middleware.
5. pkg/query-service/constants/constants.go:155
- Draft comment:
TheGetEvalDelayfunction correctly usesGetOrDefaultEnvto fetch environment variables, which is a good practice for configuration management. - Reason this comment was not posted:
Confidence changes required:10%
TheGetEvalDelayfunction inpkg/query-service/constants/constants.gousesGetOrDefaultEnvto fetch environment variables. This is a good practice for configuration management.
6. pkg/query-service/app/server.go:265
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is applicable in other files as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_aEx3xjU2X7azdOVS
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.
👍 Looks good to me! Incremental review on 47a517c in 32 seconds
More details
- Looked at
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/signoz/config.go:73
- Draft comment:
Ensure the environment variable value forCONTEXT_TIMEOUTis a valid duration string before appending 's'. Appending 's' assumes the value is in seconds, which may not be the case if the value already includes a time unit. - Reason this comment was not posted:
Comment did not seem useful.
2. pkg/signoz/config.go:83
- Draft comment:
Ensure the environment variable value forCONTEXT_TIMEOUT_MAX_ALLOWEDis a valid duration string before appending 's'. Appending 's' assumes the value is in seconds, which may not be the case if the value already includes a time unit. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_odYvTkNFEDByDM66
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.
❌ Changes requested. Incremental review on 6367d42 in 52 seconds
More details
- Looked at
285lines of code in12files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/signoz/signoz.go:70
- Draft comment:
TheAPIServerfield in theSigNozstruct is no longer used and should be removed to avoid confusion and potential errors. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
2. pkg/signoz/signoz.go:73
- Draft comment:
TheAPIServerfield in theSigNozstruct is no longer used and should be removed to avoid confusion and potential errors. - Reason this comment was not posted:
Marked as duplicate.
3. pkg/apiserver/config.go:37
- Draft comment:
TheEnabledfield in theConfigstruct is not used and should be removed. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_T0kuF5TMV8YojOnp
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. 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.
👍 Looks good to me! Incremental review on 17b00f7 in 31 seconds
More details
- Looked at
151lines of code in4files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. deploy/docker-swarm/clickhouse-setup/docker-compose.yaml:130
- Draft comment:
The PR description mentions using a common timeout middleware, but the changes here are primarily version updates. Ensure the description accurately reflects the changes made. - Reason this comment was not posted:
Confidence changes required:50%
The PR updates the image versions for several services in the docker-compose files. This is a straightforward change, but it's important to ensure that the new versions are compatible and do not introduce any breaking changes. The PR description mentions using a common timeout middleware, but the changes here are primarily version updates. The description might be misleading or incomplete. However, the version updates themselves seem consistent across the files.
2. deploy/docker-swarm/clickhouse-setup/docker-compose.yaml:133
- Draft comment:
Avoid using thecomponent/index.tsxfile 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:
Confidence changes required:0%
The PR does not introduce any new inline styles or hardcoded color values, nor does it modify the ClickHouseReader interface. The file structure is not using thecomponent/index.tsxapproach. The changes are primarily version updates in Docker Compose files.
Workflow ID: wflow_1GmgSbIDvpEzOnWT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
grandwizard28
requested changes
Jan 22, 2025
|
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.
👍 Looks good to me! Incremental review on 0c0050e in 57 seconds
More details
- Looked at
189lines of code in6files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. pkg/signoz/config.go:72
- Draft comment:
The environment variableCONTEXT_TIMEOUTis deprecated. Consider usingSIGNOZ_APISERVER_TIMEOUT_DEFAULTinstead, as per the updated configuration structure. - 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 that CONTEXT_TIMEOUT is deprecated, it suggests the wrong replacement variable name. The code clearly shows in the error message that SIGNOZ_APISERVER_CONTEXT_TIMEOUT is the new variable, not SIGNOZ_APISERVER_TIMEOUT_DEFAULT. Providing incorrect information is worse than no information at all.
The comment is about a real change in the code, and deprecation notices can be helpful. Maybe we should keep it but fix the variable name?
No - the code itself already prints a deprecation notice with the correct new variable name. The comment would be redundant even if it were correct.
Delete the comment because it provides incorrect information and is redundant with the built-in deprecation notice in the code.
2. pkg/signoz/config.go:82
- Draft comment:
The environment variableCONTEXT_TIMEOUT_MAX_ALLOWEDis deprecated. Consider usingSIGNOZ_APISERVER_TIMEOUT_MAXinstead, as per the updated configuration structure. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code shows that CONTEXT_TIMEOUT_MAX_ALLOWED is indeed deprecated, but the comment suggests the wrong replacement variable name. The actual replacement shown in the code's print statement is SIGNOZ_APISERVER_CONTEXT_TIMEOUT_MAX_ALLOWED, not SIGNOZ_APISERVER_TIMEOUT_MAX. This makes the comment misleading and potentially harmful.
The comment is about a real deprecation, which suggests it might be valuable information. The code does show a deprecation notice being printed.
However, the deprecation notice is already printed in the code itself at line 81, making this comment redundant. More importantly, the comment provides incorrect information about the new variable name.
Delete the comment because it provides incorrect information about the replacement variable name, and the deprecation notice is already handled by the code's built-in print statement.
3. pkg/query-service/app/server.go:264
- Draft comment:
Avoid using thecomponent/index.tsxfile 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 not on a valid diff hunk.
4. ee/query-service/app/server.go:318
- Draft comment:
Avoid using thecomponent/index.tsxfile 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_EGgxdRhO65t04MJN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
grandwizard28
approved these changes
Jan 22, 2025
grandwizard28
requested changes
Jan 22, 2025
|
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.
❌ Changes requested. Incremental review on 56a566c in 38 seconds
More details
- Looked at
61lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. pkg/apiserver/config.go:9
- Draft comment:
Avoid using thecomponent/index.tsxfile 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:
Confidence changes required:0%
The changes in the PR do not violate any of the specified rules. The changes are related to configuration and do not involve any hardcoded colors, inline styles, or inappropriate file structures.
Workflow ID: wflow_eIRW7jLlpW7gHemS
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6f2450a in 15 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/signoz/config.go:87
- Draft comment:
The error message was updated to correctly referenceCONTEXT_TIMEOUT_MAX_ALLOWED, which matches the environment variable being checked. This change is correct and improves clarity. - Reason this comment was not posted:
Confidence changes required:0%
The change in the error message is consistent with the environment variable being checked. The previous message was incorrect.
2. pkg/signoz/config.go:87
- Draft comment:
No issues found related to the specified rules. - Reason this comment was not posted:
Confidence changes required:0%
The code does not violate any of the specified rules. The change is a simple string update in a log message, which is not related to any of the rules about design tokens, inline styles, ClickHouseReader interface, or file structure.
Workflow ID: wflow_qrR0mPcE8ZSvq6Ni
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> |
grandwizard28
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of https://github.com/SigNoz/platform-pod/issues/406
Important
Replaces custom timeout middleware with a common one, updates configurations, and adjusts tests accordingly.
setTimeoutMiddlewarewithmiddleware.NewTimeoutinserver.goinee/query-service/appandpkg/query-service/app.setTimeoutMiddlewareand related functions fromserver.goinee/query-service/appandpkg/query-service/app.Configstruct inpkg/apiserver/config.gofor timeout settings.example.yamlwithapiserver.timeoutsettings.CONTEXT_TIMEOUTandCONTEXT_TIMEOUT_MAX_ALLOWEDinpkg/signoz/config.go.config_test.goinpkg/apiserverto test environment variable configuration.timeout_test.goinpkg/http/middlewareto test new timeout middleware behavior.This description was created by
for 6f2450a. It will automatically update as commits are pushed.