-
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
feat: docker/swarm deployment restructuring and improvements #6904
Conversation
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
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.
❌ Changes requested. Reviewed everything up to 3abaa12 in 2 minutes and 4 seconds
More details
- Looked at
5250
lines of code in62
files - Skipped
2
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. ee/query-service/integrations/signozio/signozio.go:48
- 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. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_UpT0N8dRmNJFNauj
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.
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 8da569a in 1 minute and 7 seconds
More details
- Looked at
1591
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_HDW6s0uOAa55b3hX
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> |
Signed-off-by: Prashant Shahi <[email protected]>
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 c703e25 in 46 seconds
More details
- Looked at
31
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. .github/workflows/ci.yaml:17
- Draft comment:
Renaming the job fromtest-frontend
tobuild-frontend
is appropriate as the job includes build steps likeyarn tsc
. This improves clarity. - Reason this comment was not posted:
Confidence changes required:0%
The PR changes job names from 'test-' to 'build-', which aligns with the actions being performed in the jobs. This is a good change for clarity.
2. .github/workflows/ci.yaml:32
- Draft comment:
Renaming the job fromtest-query-service
tobuild-query-service
is appropriate as the job includes build steps likemake build-query-service-amd64
. This improves clarity. - Reason this comment was not posted:
Confidence changes required:0%
The PR changes job names from 'test-' to 'build-', which aligns with the actions being performed in the jobs. This is a good change for clarity.
3. .github/workflows/ci.yaml:50
- Draft comment:
Renaming the job fromtest-ee-query-service
tobuild-ee-query-service
is appropriate as the job includes build steps likemake build-ee-query-service-amd64
. This improves clarity. - Reason this comment was not posted:
Confidence changes required:0%
The PR changes job names from 'test-' to 'build-', which aligns with the actions being performed in the jobs. This is a good change for clarity.
4. .github/workflows/ci.yaml:17
- Draft comment:
The changes in the workflow file are appropriate and do not violate any of the specified rules. - Reason this comment was not posted:
Confidence changes required:0%
The changes in the workflow file are appropriate and do not violate any of the specified rules.
Workflow ID: wflow_LPYr64v5wPuZWBGc
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> |
Signed-off-by: Prashant Shahi <[email protected]>
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Signed-off-by: Prashant Shahi <[email protected]>
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 62b4aeb in 1 minute and 14 seconds
More details
- Looked at
226
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. deploy/docker-swarm/generator/infra/docker-compose.yaml:17
- Draft comment:
Usinguser: root
can pose security risks. Consider using a non-root user if possible. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The services require access to docker.sock and filesystem paths which generally need root access. The otel-agent needs to monitor the host system and logspout needs to access container logs. These are infrastructure monitoring components where root access is often necessary. A non-root user would likely break functionality.
The comment raises a valid security best practice in general. Maybe there's a way to use more restricted permissions or capabilities instead of full root access.
While using non-root is generally good practice, these specific services are infrastructure components that require elevated privileges to function. There isn't a practical alternative here.
Delete the comment because root access appears necessary for these monitoring services to function properly. The suggestion isn't actionable in this context.
2. deploy/docker/generator/infra/docker-compose.yaml:14
- Draft comment:
Usinguser: root
can pose security risks. Consider using a non-root user if possible. - Reason this comment was not posted:
Marked as duplicate.
3. deploy/docker/generator/infra/docker-compose.yaml:32
- Draft comment:
Avoid usingcontainer_name
as it can lead to conflicts if multiple instances are run. It's better to let Docker handle container naming. - Reason this comment was not posted:
Comment was on unchanged code.
4. deploy/docker-swarm/generator/hotrod/docker-compose.yaml:15
- Draft comment:
Avoid usingcontainer_name
as it can lead to conflicts if multiple instances are run. It's better to let Docker handle container naming. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_23MGxEeL1cggDRE3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Prashant Shahi <[email protected]>
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 5eda481 in 1 minute and 7 seconds
More details
- Looked at
280
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. deploy/docker-swarm/generator/infra/docker-compose.yaml:33
- Draft comment:
Usinguser: 0:0
can be a security risk. Consider using a specificUID:GID
with necessary permissions instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment essentially repeats what's already documented in the inline comment. The code already acknowledges the security concern and provides the mitigation strategy. The inline comment is actually more helpful as it explains why elevated permissions might be needed (for docker.sock access). The automated comment doesn't add any new information.
The security concern is legitimate and important. Maybe keeping both the inline comment and the review comment would emphasize the importance of this security consideration.
While security is important, duplicating the warning doesn't add value. The inline comment is more specific and actionable, making the review comment redundant.
Delete the comment since it's redundant with the existing inline comment that already provides better context and mitigation strategy.
2. deploy/docker-swarm/generator/infra/docker-compose.yaml:22
- Draft comment:
The environment variables and configurations forotel-agent
andotel-metrics
are very similar. Consider refactoring to avoid duplication. - Reason this comment was not posted:
Confidence changes required:50%
The otel-agent and otel-metrics services have similar environment variables and configurations. Consider refactoring to avoid duplication.
3. deploy/docker-swarm/generator/infra/otel-agent-config.yaml:45
- Draft comment:
The filter excludes logs from certain containers. Ensure this is documented or configurable if needed. - Reason this comment was not posted:
Confidence changes required:50%
The otel-agent-config.yaml file has a filter that excludes certain container names from log collection. This should be documented or configurable.
4. deploy/docker-swarm/generator/infra/otel-agent-config.yaml: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. This applies to other files in this PR as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_CFRGIX83tBvBvZCn
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Prashant Shahi <[email protected]>
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 b113170 in 41 seconds
More details
- Looked at
39
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. deploy/docker-swarm/otel-collector-config.yaml:75
- Draft comment:
Remove the commented-out debug exporter if it's not needed to keep the configuration clean. - Reason this comment was not posted:
Confidence changes required:50%
The commented-out debug exporter in the otel-collector-config.yaml file should be removed if not needed, as it clutters the configuration file.
2. deploy/docker-swarm/docker-compose.ha.yaml:234
- Draft comment:
Ensure that the system can handle the load of 3 replicas for theotel-collector
service. - Reason this comment was not posted:
Confidence changes required:33%
Thereplicas: 3
setting for theotel-collector
service in bothdocker-compose.ha.yaml
anddocker-compose.yaml
files is a good practice for high availability. However, ensure that the system can handle the load of multiple replicas.
3. deploy/docker-swarm/docker-compose.yaml:170
- Draft comment:
Ensure that the system can handle the load of 3 replicas for theotel-collector
service. - Reason this comment was not posted:
Confidence changes required:33%
Thereplicas: 3
setting for theotel-collector
service in bothdocker-compose.ha.yaml
anddocker-compose.yaml
files is a good practice for high availability. However, ensure that the system can handle the load of multiple replicas.
4. deploy/docker-swarm/otel-collector-config.yaml:75
- Draft comment:
Consider removing the commented-out debug exporter if it's not needed. - Reason this comment was not posted:
Confidence changes required:33%
The PR contains a commented-out debug exporter in the otel-collector-config.yaml file, which is not a violation of the rules but could be considered for removal if not needed.
Workflow ID: wflow_tCxgzuUMhNeY0Fzm
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> |
Signed-off-by: Prashant Shahi <[email protected]>
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
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 35c7489 in 57 seconds
More details
- Looked at
421
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. deploy/install.sh:230
- Draft comment:
Thehas_wget
function is defined but never used. Consider removing it to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
The functionhas_wget
is defined but not used anywhere in the script. This is likely an oversight and should be removed to clean up the code.
2. deploy/install.sh:535
- Draft comment:
Remove trailing spaces in echo statements for cleaner code. - Reason this comment was not posted:
Confidence changes required:50%
There are trailing spaces in the echo statements. These should be removed to maintain code cleanliness.
3. deploy/install.sh:556
- Draft comment:
Remove trailing spaces in echo statements for cleaner code. - Reason this comment was not posted:
Confidence changes required:50%
There are trailing spaces in the echo statements. These should be removed to maintain code cleanliness.
4. deploy/install.sh:535
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values for consistency. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_nYLCyheP9aQvkpTR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Prashant Shahi <[email protected]>
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 4c8e219 in 50 seconds
More details
- Looked at
24
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. deploy/docker-swarm/docker-compose.ha.yaml:217
- Draft comment:
Removingcontainer_name
can lead to issues if any scripts or configurations rely on the specific namesignoz-frontend
. Ensure no dependencies on this name exist. - Reason this comment was not posted:
Confidence changes required:50%
The removal of thecontainer_name
directive in thefrontend
service is a notable change. This could lead to issues if any scripts or configurations rely on the specific container namesignoz-frontend
. However, if the intent is to allow Docker to automatically generate a container name, this change is valid. It's important to ensure that no dependencies on the container name exist elsewhere in the codebase or deployment scripts.
2. deploy/docker-swarm/docker-compose.yaml:153
- Draft comment:
Removingcontainer_name
can lead to issues if any scripts or configurations rely on the specific namesignoz-frontend
. Ensure no dependencies on this name exist. - Reason this comment was not posted:
Confidence changes required:50%
The removal of thecontainer_name
directive in thefrontend
service is a notable change. This could lead to issues if any scripts or configurations rely on the specific container namesignoz-frontend
. However, if the intent is to allow Docker to automatically generate a container name, this change is valid. It's important to ensure that no dependencies on the container name exist elsewhere in the codebase or deployment scripts.
3. deploy/docker-swarm/docker-compose.ha.yaml:215
- 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. This applies to thefrontend
service in this file and thefrontend
service indocker-compose.yaml
. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_msMkUGd8N098hw2o
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Prashant Shahi <[email protected]>
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 e893ff9 in 56 seconds
More details
- Looked at
366
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. .gitpod.yml:9
- Draft comment:
The commandsudo docker compose up -d
is correctly updated to reflect the new directory structure. Ensure that thedocker-compose.yaml
file is present in thedeploy/docker
directory. - Reason this comment was not posted:
Confidence changes required:20%
The PR description mentions restructuring and improvements for Docker/Swarm deployment. The changes in the .gitpod.yml file reflect this by updating the Docker Compose command to use the new directory structure. This aligns with the intent of the PR.
2. Makefile:18
- Draft comment:
The STANDALONE_DIRECTORY path has been updated todeploy/docker
, which aligns with the restructuring mentioned in the PR description. Ensure that the directory structure is consistent across the project. - Reason this comment was not posted:
Confidence changes required:20%
The PR description indicates a restructuring of the Docker/Swarm deployment directories. The changes in the Makefile reflect this by updating the STANDALONE_DIRECTORY and SWARM_DIRECTORY paths. This aligns with the intent of the PR.
3. deploy/README.md:25
- Draft comment:
The commandcd deploy/docker
is correctly updated to reflect the new directory structure. Ensure that thedocker-compose.yaml
file is present in thedeploy/docker
directory. - Reason this comment was not posted:
Confidence changes required:20%
The PR description mentions removing Kubernetes HOTRod files and restructuring Docker/Swarm deployment. The changes in the deploy/README.md file reflect this by updating the instructions to use the new directory structure and removing Kubernetes references. This aligns with the intent of the PR.
4. frontend/README.md:9
- Draft comment:
The commanddocker compose up
is correctly updated to reflect the new directory structure. Ensure that thedocker-compose.yaml
file is present in thedeploy/docker
directory. - Reason this comment was not posted:
Confidence changes required:20%
The PR description mentions restructuring Docker/Swarm deployment directories. The changes in the frontend/README.md file reflect this by updating the Docker Compose command to use the new directory structure. This aligns with the intent of the PR.
5. frontend/README.md:9
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This comment applies to other instances of inline styles in the code as well. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_B17q4Ns5nA42zZzC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Prashant Shahi <[email protected]>
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 428d8a6 in 57 seconds
More details
- Looked at
218
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. .github/workflows/push.yaml:110
- Draft comment:
The workflow still includes steps for building and pushing the frontend images, which seems inconsistent with the stated intent of deprecating frontend and CI-related tasks. Consider removing these steps if they are no longer needed. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ZaWaVR5H9IPaJ1zn
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Prashant Shahi <[email protected]>
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Signed-off-by: Prashant Shahi <[email protected]>
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 ad57e43 in 1 minute and 9 seconds
More details
- Looked at
86
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/Dockerfile:14
- Draft comment:
You should include a step to install dependencies, such asRUN npm install
, before copying the build files. This ensures all necessary packages are available at runtime. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
This is a multi-stage build pattern where the build artifacts are created separately and only the final nginx stage is shown here. The comment shows a misunderstanding of the container's purpose - it's meant to serve static files, not build the application. npm dependencies are not needed at runtime for serving static files.
Could there be runtime dependencies that actually do need to be installed in the nginx container? Could the build directory contents require some node modules?
No - this is a standard nginx static file server. The build artifacts are complete and self-contained, requiring only nginx to serve them. Any runtime dependencies would be bundled into the build files already.
The comment should be deleted as it shows a misunderstanding of the container's purpose and would introduce unnecessary bloat by installing npm packages that aren't needed for serving static files.
2. frontend/docker-compose.yaml:7
- Draft comment:
Consider adding a volume mapping for the frontend build files to ensure they are included in the container. For example:
volumes:
- ./build:/usr/share/nginx/html
- Reason this comment was not posted:
Comment was not on a valid diff hunk.
3. frontend/docker-compose.yaml: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. This applies to the overall structure of the frontend directory. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_yeuOOxoZYkJA5Lh2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Prashant Shahi <[email protected]>
Summary
This PR consists of improvements and fixes related to SigNoz in Docker Standalone and Docker Swarm.
Frontend deprecation and CI related-tasksImportant
Restructures Docker and Swarm deployments for SigNoz, enhances Prometheus metrics scraping, and unifies schema migration handling.
install.sh
to reflect new directory structure.histogram-quantile
from release instead of local binary mount.Makefile
to removerun-local
anddown-local
targets.CONTRIBUTING.md
andREADME.md
for new setup instructions.This description was created by for ad57e43. It will automatically update as commits are pushed.