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

chore: add count based limits for metrics #6738

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Jan 1, 2025

Fixes https://github.com/SigNoz/engineering-pod/issues/1951

Summary

Related Issues / PR's

Screenshots

NA

Affected Areas and Manually Tested Areas


Important

Add count-based limits for metrics in MultiIngestionSettings.tsx, updating UI and types to support this feature.

  • Behavior:
    • Add count-based limits for metrics in MultiIngestionSettings.tsx.
    • Introduce COUNT_MULTIPLIER and SIGNALS_CONFIG to handle count-based limits.
    • Update handleAddLimit and handleUpdateLimit to process count limits.
  • UI Changes:
    • Update forms in MultiIngestionSettings.tsx to include count-based fields.
    • Display count limits in the UI alongside size limits.
  • Types:
    • Add count field to LimitConfig in types.ts.
    • Update LimitSettings, AddLimitProps, and UpdateLimitProps to support count limits.

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

@github-actions github-actions bot added the chore label Jan 1, 2025
Copy link

github-actions bot commented Jan 1, 2025

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

@srikanthccv srikanthccv marked this pull request as ready for review January 2, 2025 13:24
@srikanthccv srikanthccv requested a review from YounixM as a code owner January 2, 2025 13:24
Copy link

github-actions bot commented Jan 2, 2025

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.

❌ Changes requested. Reviewed everything up to ea9bcc5 in 3 minutes and 9 seconds

More details
  • Looked at 968 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:58
  • Draft comment:
    The PR description mentions removing the 'Beta' tag from logs in the sidebar, but the code changes do not address this issue. The changes are related to adding count-based limits for metrics, which is unrelated to the issue described.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_Fi3GaFSmluXbjzq4


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.

{/* Count (if usesCount) */}
{signalCfg.usesCount &&
(limit?.config?.day?.count !== undefined ? (
<div style={{ marginTop: 4 }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using inline styles. Use external stylesheets, CSS classes, or styled components instead. This issue is also present at line 1197.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant