Skip to content
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(ee): serve frontend pages with query-service #6696

Merged
merged 7 commits into from
Dec 23, 2024

Conversation

grandwizard28
Copy link
Collaborator

@grandwizard28 grandwizard28 commented Dec 22, 2024

Summary

Serve frontend pages with query-service

Related Issues / PR's

Screenshots

NA

Affected Areas and Manually Tested Areas


Important

Serve frontend pages using query-service by integrating web package and updating Docker and configuration files.

  • Behavior:
    • Serve frontend pages using query-service by integrating web package in server.go and main.go.
    • New configuration defaults in defaults.yaml for web serving.
  • Docker:
    • Update Dockerfile to copy frontend build to /etc/signoz/web/.
  • Build:
    • Modify Makefile to include build-frontend-static in build-push-ee-query-service target.
  • Configuration:
    • Add web configuration to config.go and unmarshaler.go.
    • Update unmarshaler_test.go to test instrumentation config unmarshalling.
  • CI/CD:
    • Add step in push.yaml to create .env file with secrets for frontend.

This description was created by Ellipsis for 5b52fdd. It will automatically update as commits are pushed.

@grandwizard28 grandwizard28 requested a review from a team as a code owner December 22, 2024 07:16
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the enhancement New feature or request label Dec 22, 2024
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 005822e in 1 minute and 12 seconds

More details
  • Looked at 280 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. ee/query-service/app/server.go:111
  • Draft comment:
    The web parameter is not used in the NewServer function. If it's not needed, consider removing it to avoid confusion.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. ee/query-service/app/server.go:342
  • Draft comment:
    The web parameter is not used in the createPublicServer function. If it's not needed, consider removing it to avoid confusion.
  • Reason this comment was not posted:
    Marked as duplicate.
3. ee/query-service/main.go:189
  • Draft comment:
    The web parameter is not used in the NewServer function. If it's not needed, consider removing it to avoid confusion.
  • Reason this comment was not posted:
    Marked as duplicate.
4. ee/query-service/app/server.go:293
  • Draft comment:
    The web parameter is not used in the createPublicServer function. If it's not needed, consider removing it to avoid confusion.
  • Reason this comment was not posted:
    Marked as duplicate.
5. ee/query-service/Dockerfile:27
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values to maintain consistency in design and theming. This applies to the frontend build files being copied here.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_ZojOiSXDToIOAxFp


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 3d002f3 in 35 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. Makefile:131
  • Draft comment:
    The build-push-ee-query-service target no longer depends on build-frontend-static. Consider adding it back if the frontend static files are required for the EE query service docker image.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment appears to be raising a concern about a missing dependency, but it's actually a false alarm. The dependency wasn't removed - it was just moved to a different target that's still in the dependency chain. build-push-ee-query-service calls build-push-query-service which depends on build-query-service-static-all which now includes build-frontend-static.
    I could be wrong about the dependency chain - there might be some make variables or conditional execution that changes the behavior in ways not obvious from just reading the Makefile.
    The dependency chain is clear in the Makefile, and the frontend static files will still be built as part of the overall process, just at a different stage.
    The comment should be deleted because it raises a false concern - the frontend static files are still being built as part of the dependency chain, just at a different stage.
2. Makefile:101
  • Draft comment:
    Avoid using inline commands for building frontend static files. Consider using a separate script or configuration file to manage these commands.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_celGKh34zJA3njiM


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 5b52fdd in 12 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. Makefile:106
  • Draft comment:
    Ensure that build-frontend-static is included in all relevant targets that depend on frontend assets. This change is correctly applied here, but verify other targets like build-push-query-service and build-query-service-static-all for consistency.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change in the Makefile is to include build-frontend-static in the build-query-service-amd64 target. This ensures that the frontend static files are built before the query service docker image is built. This change seems correct given the PR description, but I need to ensure that this change is consistently applied across similar targets.
2. Makefile:106
  • Draft comment:
    Avoid using the component/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:
    Confidence changes required: 0%
    The Makefile does not have any issues related to the rules provided. It does not contain any inline styles, hardcoded colors, or inappropriate file structures. The changes made are consistent with the existing structure and do not introduce any new issues.

Workflow ID: wflow_HOaSZNgAa2jqGhPc


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@grandwizard28 grandwizard28 merged commit 53b86e4 into main Dec 23, 2024
16 of 18 checks passed
@grandwizard28 grandwizard28 deleted the feat/frontend+query-service branch December 23, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants