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: added support for instrumentation scope in logs #6378

Merged
merged 6 commits into from
Nov 7, 2024

Conversation

vikrantgupta25
Copy link
Collaborator

@vikrantgupta25 vikrantgupta25 commented Nov 6, 2024

Summary

  • added instrumentation scope support in logs module

Related Issues / PR's

contributes to - #5442

Screenshots

Screen.Recording.2024-11-06.at.2.34.32.PM.mov

Affected Areas and Manually Tested Areas

  • logs query builder
  • logs details drawer

Important

Adds support for instrumentation scope in logs, updating data structures, styles, and log processing functions.

  • Behavior:
    • Added scope_string to ILog interface in log.ts.
    • Updated aggregateAttributesResourcesToString and getFieldAttributes in utils.tsx to handle scope_string.
  • Styles:
    • Added .scope styles in QueryBuilderSearchV2.styles.scss and Suggestions.styles.scss for visual differentiation.
  • Misc:
    • Added Scope to MetricsType enum in constant.ts.

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

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

github-actions bot commented Nov 6, 2024

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

SagarRajput-7
SagarRajput-7 previously approved these changes Nov 6, 2024
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 ba80cdf in 34 seconds

More details
  • Looked at 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/container/LogDetailedView/TableView.tsx:64
  • Draft comment:
    Remove unnecessary console.log statements for cleaner production code.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/container/LogDetailedView/TableView.tsx:64
  • Draft comment:
    Avoid using inline styles. Consider using a CSS class or styled component instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_aZJ1XIjLFOaGuVk5


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.

❌ Changes requested. Reviewed everything up to 448dd51 in 6 minutes and 9 seconds

More details
  • Looked at 153 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/container/LogDetailedView/TableView.tsx:67
  • Draft comment:
    Remove console.log statements used for debugging.
	```
  • Reason this comment was not posted:
    Marked as duplicate.
2. frontend/src/container/LogDetailedView/TableView.tsx:183
  • Draft comment:
    Remove console.log statements used for debugging.
	```
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_lOLNU9uz8OLmA653


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.

Copy link
Member

@nityanandagohain nityanandagohain left a comment

Choose a reason for hiding this comment

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

The group by caluse renders the prefix as well. while the query runs please remove the prefix

image

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. Incremental review on 2359959 in 1 minute and 16 seconds

More details
  • Looked at 220 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/AggregatorFilter/AggregatorFilter.tsx:84
  • Draft comment:
    The condition !item.isColumn && item.type ? item.type : '' is redundant since removePrefix already checks for the type. Simplify by directly passing item.type.
  • Reason this comment was not posted:
    Marked as duplicate.
2. frontend/src/container/QueryBuilder/filters/GroupByFilter/GroupByFilter.tsx:78
  • Draft comment:
    The condition !item.isColumn && item.type ? item.type : '' is redundant since removePrefix already checks for the type. Simplify by directly passing item.type.
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/index.tsx:410
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable in other files where inline styles are used.
  • Reason this comment was not posted:
    Comment was on unchanged code.
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/index.tsx:413
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable in other files where inline styles are used.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_9r0rpMmsQOsaY4dT


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.

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 e733577 in 12 seconds

More details
  • Looked at 23 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/GroupByFilter/utils.ts:2
  • Draft comment:
    The import of isEmpty from lodash-es is unnecessary and can be removed to clean up the code.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The import of isEmpty from lodash-es is unnecessary as it is not used in the code.
2. frontend/src/container/QueryBuilder/filters/GroupByFilter/utils.ts:14
  • Draft comment:
    The code looks good and follows the provided rules. No changes needed.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The code looks fine and follows the rules provided. No hardcoded colors, inline styles, or incorrect file structures are present.

Workflow ID: wflow_BkAV9JHModqAHbbh


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

@vikrantgupta25
Copy link
Collaborator Author

The group by caluse renders the prefix as well. while the query runs please remove the prefix

done @nityanandagohain

Copy link
Contributor

@SagarRajput-7 SagarRajput-7 left a comment

Choose a reason for hiding this comment

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

lgtm

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.

I didn't test functionality. Approving to unblock as change in MetricsApplication made me reviewer.

@SagarRajput-7 SagarRajput-7 merged commit 4718031 into develop Nov 7, 2024
12 of 13 checks passed
@SagarRajput-7 SagarRajput-7 deleted the instrumentation-scope branch November 7, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs not required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants