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: sentry error object and release management #2364

Closed
wants to merge 2 commits into from

Conversation

liorzam
Copy link
Collaborator

@liorzam liorzam commented May 9, 2024

PR Type

enhancement, bug_fix


Description

  • Added request body to the metadata object in request.ts for better request tracking.
  • Refactored ValidationError in errors.ts to use the response object directly, removing unnecessary properties.
  • Enhanced Sentry module in sentry.module.ts to include distribution version (_dist) in its setup.
  • Improved error logging in Sentry service by setting the error object in the scope.
  • Updated GitHub Actions workflows to re-enable Docker image scanning and refine Helm chart updates.
  • Modified Dockerfile to include SHORT_SHA for better traceability of builds.

Changes walkthrough 📝

Relevant files
Enhancement
request.ts
Include request body in metadata object                                   

services/workflows-service/src/common/utils/request-response/request.ts

  • Added body to the metadata object returned by getReqMetadataObj.
  • +1/-0     
    errors.ts
    Refactor ValidationError to use response object directly 

    services/workflows-service/src/errors.ts

  • Removed redundant errors property from ValidationError.
  • Modified getErrors to retrieve errors directly from the response
    object.
  • +1/-3     
    sentry.module.ts
    Extend Sentry module with distribution version management

    services/workflows-service/src/sentry/sentry.module.ts

  • Added _dist property to manage distribution version.
  • Included dist in Sentry configuration setup.
  • +3/-0     
    sentry.service.ts
    Improve error logging in Sentry service                                   

    services/workflows-service/src/sentry/sentry.service.ts

    • Enhanced error logging by adding error object to Sentry scope.
    +2/-0     
    Dockerfile
    Include SHORT_SHA in Dockerfile for build reproducibility

    services/workflows-service/Dockerfile

  • Added SHORT_SHA argument and environment variable to Dockerfile for
    both development and production stages.
  • +8/-1     
    Configuration changes
    publish-workflows-service.yml
    Update CI/CD workflows for enhanced deployment and security scanning

    .github/workflows/publish-workflows-service.yml

  • Re-enabled Docker image scanning with Trivy.
  • Updated Helm chart update logic to handle different deployment
    scenarios.
  • Adjusted release job to include a suffix for dev deployments.
  • +56/-37 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Summary by CodeRabbit

    • New Features

      • Added a condition for updating the HelmChart based on a specific requirement.
      • Introduced a suffix in the Release step under certain conditions.
    • Bug Fixes

      • Modified the getErrors method in the ValidationError class for accurate error retrieval.
    • Refactor

      • Adjusted the getReqMetadataObj function to include the body field for improved metadata handling in requests.
      • Enhanced error reporting in Sentry by setting extra data in the Sentry scope in the captureHttpException method.
    • Chores

      • Updated Docker configurations to include SHORT_SHA and RELEASE arguments for better traceability.
      • Optimized GitHub Actions workflows for improved CI/CD reliability.

    Copy link

    changeset-bot bot commented May 9, 2024

    ⚠️ No Changeset found

    Latest commit: 32e544b

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    Copy link
    Contributor

    coderabbitai bot commented May 9, 2024

    Walkthrough

    The recent update enriches the workflows-service by refining Docker configurations, error handling, and improving request logging metadata. Noteworthy changes include the addition of SHORT_SHA for traceability, conditional logic in Helm chart updates, and enhancements for better error reporting insights.

    Changes

    File Path Change Summary
    .github/workflows/.../publish-workflows-service.yml Docker build args updated, image scan parameters changed, Helm chart paths adjusted, release steps updated.
    services/.../Dockerfile Added SHORT_SHA, modified RELEASE setup.
    services/.../src/common/.../request.ts Enhanced request metadata with body field.
    services/.../src/errors.ts Improved error method for better error detail retrieval.
    services/.../src/sentry/sentry.module.ts Integrated SHORT_SHA into Sentry for version tracking.
    services/.../src/sentry/sentry.service.ts Enhanced Sentry logging with additional error details.

    🐇✨
    In the garden of code, changes bloom bright,
    A rabbit hops through, with delight.
    SHORT_SHA in place, errors in trace,
    Helm charts sail smoothly in cyberspace.
    Every commit, a step to enhance,
    In this tech maze, we dance the advance.
    🌟🚀


    Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

    Share
    Tips

    Chat

    There are 3 ways to chat with CodeRabbit:

    • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
      • I pushed a fix in commit <commit_id>.
      • Generate unit testing code for this file.
      • Open a follow-up GitHub issue for this discussion.
    • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
      • @coderabbitai generate unit testing code for this file.
      • @coderabbitai modularize this function.
    • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
      • @coderabbitai generate interesting stats about this repository and render them as a table.
      • @coderabbitai show all the console.log statements in this repository.
      • @coderabbitai read src/utils.ts and generate unit testing code.
      • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

    Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

    CodeRabbit Commands (invoked as PR comments)

    • @coderabbitai pause to pause the reviews on a PR.
    • @coderabbitai resume to resume the paused reviews.
    • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
    • @coderabbitai resolve resolve all the CodeRabbit review comments.
    • @coderabbitai help to get help.

    Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

    CodeRabbit Configration File (.coderabbit.yaml)

    • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
    • Please see the configuration documentation for more information.
    • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

    Documentation and Community

    • Visit our Documentation for detailed information on how to use CodeRabbit.
    • Join our Discord Community to get help, request features, and share feedback.
    • Follow us on X/Twitter for updates and announcements.

    @github-actions github-actions bot added enhancement New feature or request bug_fix labels May 9, 2024
    Copy link
    Contributor

    github-actions bot commented May 9, 2024

    PR Description updated to latest commit (659104b)

    Copy link
    Contributor

    github-actions bot commented May 9, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and changes across different layers including Docker configurations, Sentry setup, and error handling. The changes are significant but not overly complex, requiring a moderate level of understanding of the project's architecture and technologies used.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The removal of this.errors in ValidationError and the subsequent change to use (this.getResponse() as any).errors might lead to runtime errors if getResponse() does not return an object with an errors property.

    Data Exposure: Adding req.body to the metadata object in request.ts could potentially log sensitive information if not properly sanitized.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileservices/workflows-service/src/common/utils/request-response/request.ts
    suggestion      

    Consider sanitizing or filtering sensitive information from req.body before adding it to the metadata object to prevent potential data exposure. [important]

    relevant linebody: req.body,

    relevant fileservices/workflows-service/src/errors.ts
    suggestion      

    Ensure that getResponse() always returns an object containing an errors property or implement error handling to manage cases where it does not. This change is crucial to avoid runtime errors. [important]

    relevant linereturn (this.getResponse() as any).errors;

    relevant fileservices/workflows-service/src/sentry/sentry.module.ts
    suggestion      

    Validate or check the existence of _dist before using it in the Sentry configuration to ensure it does not break Sentry's setup if SHORT_SHA is undefined. [medium]

    relevant linedist: this._dist,

    relevant file.github/workflows/publish-workflows-service.yml
    suggestion      

    Ensure that the SHORT_SHA environment variable is consistently available or handle cases where it might be missing to prevent build failures. [medium]

    relevant line"SHORT_SHA=${{ steps.version.outputs.sha_short }}"

    Copy link
    Contributor

    github-actions bot commented May 9, 2024

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add a check to ensure req.body is not empty before adding it to the metadata object.

    Consider checking if req.body is not empty or null before adding it to the metadata
    object. This can prevent sending unnecessary or sensitive data.

    services/workflows-service/src/common/utils/request-response/request.ts [30]

    -body: req.body,
    +body: req.body && Object.keys(req.body).length ? req.body : undefined,
     
    Best practice
    Replace any with a specific type to ensure type safety.

    Ensure type safety by avoiding the use of as any. Consider defining a proper interface or
    type for the expected structure of getResponse().

    services/workflows-service/src/errors.ts [63]

    -return (this.getResponse() as any).errors;
    +return (this.getResponse() as ResponseType).errors;
     
    Security
    Sanitize environment variables before using them in Sentry configuration.

    Validate or sanitize the environment variables before using them to configure Sentry, to
    avoid potential security risks or misconfigurations.

    services/workflows-service/src/sentry/sentry.module.ts [34]

    -this._dist = this.configService.get('SHORT_SHA');
    +this._dist = sanitize(this.configService.get('SHORT_SHA'));
     
    Filter sensitive data before setting it as extra data in Sentry.

    Instead of directly setting 'error' as extra data, consider checking if it contains
    sensitive information or large unnecessary data.

    services/workflows-service/src/sentry/sentry.service.ts [24]

    -scope.setExtra('error', error);
    +scope.setExtra('error', filterSensitiveData(error));
     
    Modify the Docker image scan to fail the workflow on finding vulnerabilities.

    Ensure that the Docker image scan with Trivy is not set to continue on error. This could
    lead to ignoring critical vulnerabilities.

    .github/workflows/publish-workflows-service.yml [151]

    -continue-on-error: true
    +continue-on-error: false
     

    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 3

    Review Details

    Configuration used: CodeRabbit UI
    Review profile: CHILL

    Commits Files that changed from the base of the PR and between 64ac06c and 659104b.
    Files selected for processing (6)
    • .github/workflows/publish-workflows-service.yml (4 hunks)
    • services/workflows-service/Dockerfile (2 hunks)
    • services/workflows-service/src/common/utils/request-response/request.ts (1 hunks)
    • services/workflows-service/src/errors.ts (1 hunks)
    • services/workflows-service/src/sentry/sentry.module.ts (3 hunks)
    • services/workflows-service/src/sentry/sentry.service.ts (1 hunks)
    Additional comments not posted (5)
    services/workflows-service/Dockerfile (1)

    26-29: The changes in the production stage mirror those in the development stage, ensuring consistency across builds. This is good practice as it maintains uniformity and traceability of builds using SHORT_SHA.

    services/workflows-service/src/common/utils/request-response/request.ts (1)

    30-30: Adding the body field to the metadata object can enrich logging and debugging capabilities. Ensure that sensitive data is not logged unintentionally, especially if body contains user inputs or PII.

    Verification successful

    The logging of the body field found in the file end-user.controller.external.intg.test.ts is part of an integration test. This does not pose a risk of exposing sensitive data in production environments. Therefore, the addition of the body field to the metadata object in the production code is verified to not unintentionally log sensitive data in production logs.

    • File: services/workflows-service/src/end-user/end-user.controller.external.intg.test.ts
    • Context: Logging within integration tests, which is generally acceptable and does not impact production security.
    Scripts Executed

    The following scripts were executed for the analysis:


    Script:

    #!/bin/bash
    # Description: Verify that sensitive data is not logged in the `body` field.
    
    # Test: Search for logging calls that might include `body`. Expect: No direct logging of `body` or it's sanitized.
    rg --type typescript $'console.log\\(.*body.*\\)' services/workflows-service/src
    

    Length of output: 114



    Script:

    #!/bin/bash
    # Description: Re-verify that sensitive data is not logged in the `body` field.
    
    # Correcting the previous script to search for logging calls in TypeScript files by specifying file extensions.
    rg 'console.log\(.*body.*\)' --glob '*.ts' services/workflows-service/src
    

    Length of output: 193

    services/workflows-service/src/sentry/sentry.service.ts (1)

    24-25: Adding detailed error information in the Sentry scope can significantly improve the debugging process. Ensure that no sensitive information is inadvertently exposed through error details.

    services/workflows-service/src/sentry/sentry.module.ts (1)

    Line range hint 24-45: The addition of _dist initialized with SHORT_SHA and its inclusion in Sentry's initialization enhances traceability of errors by linking them to specific builds. This is a valuable improvement for debugging and release management.

    services/workflows-service/src/errors.ts (1)

    63-63: Changing the source of errors in the getErrors method to the response object can help align error handling with external modifications or standards. Ensure that the response object is always properly formatted to avoid runtime errors.

    services/workflows-service/Dockerfile Show resolved Hide resolved
    @liorzam
    Copy link
    Collaborator Author

    liorzam commented May 12, 2024

    image https://github.com/ballerine-io/ballerine/actions/runs/9049161597 @alonp99

    Copy link
    Contributor

    @coderabbitai coderabbitai bot left a comment

    Choose a reason for hiding this comment

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

    Actionable comments posted: 0

    Review Details

    Configuration used: CodeRabbit UI
    Review profile: CHILL

    Commits Files that changed from the base of the PR and between 659104b and 32e544b.
    Files selected for processing (1)
    • services/workflows-service/src/errors.ts (1 hunks)
    Files skipped from review as they are similar to previous changes (1)
    • services/workflows-service/src/errors.ts

    @liorzam
    Copy link
    Collaborator Author

    liorzam commented May 15, 2024

    @liorzam liorzam closed this May 15, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants