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.

Sign up for GitHub

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

Jump to bottom

chore: capturing more telemetry data as per fe coder's requirements(#29153) #29287

Merged
merged 4 commits into from
Dec 5, 2023
Merged

chore: capturing more telemetry data as per fe coder's requirements(#29153) #29287

merged 4 commits into from
Dec 5, 2023

Conversation

vsvamsi1
Copy link
Contributor

@vsvamsi1 vsvamsi1 commented Dec 4, 2023 •
edited by coderabbitai bot
Loading

Description

Capturing evalTree telemetry data as well as from a few evaluation saga flows.

PR fixes following issue(s)

Fixes #29153

Type of change

  • Chore (housekeeping or task changes that don't impact user perception)

Testing

How Has This Been Tested?

  • Manual
  • JUnit
  • Jest
  • Cypress

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • PR is being merged under a feature flag

QA activity:

  • Speedbreak features have been covered
  • Test plan covers all impacted features and areas of interest
  • Test plan has been peer reviewed by project stakeholders and other QA members
  • Manually tested functionality on DP
  • We had an implementation alignment call with stakeholders post QA Round 2
  • Cypress test cases have been added and approved by SDET/manual QA
  • Added Test Plan Approved label after Cypress tests were reviewed
  • Added Test Plan Approved label after JUnit tests were reviewed

Summary by CodeRabbit

  • New Features

    • Enhanced telemetry with the ability to specify start times for nested spans.
    • Introduced web worker span management for improved performance monitoring.
    • Implemented profiling functions to trace evaluation processes.

  • Improvements

    • Added telemetry spans to key functions for better performance insights.
    • Refined control flow to include telemetry span handling in various sagas.

  • Documentation

    • Updated interface definitions to include new telemetry-related properties.

  • Refactor

    • Streamlined conditional logic to incorporate telemetry span completion.

@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 4, 2023 •
edited
Loading

Walkthrough

Walkthrough

The changes across multiple files introduce enhanced telemetry and tracing capabilities, specifically targeting the evaluation flows for frontend coders. New functions and interfaces have been added to manage and convert web worker spans, and existing functions have been instrumented with start and end span calls to provide more detailed traces. These modifications aim to improve debugging by offering more granular insights into the performance and behavior of the code during evaluations.

Changes

File Path Change Summary
app/client/src/UITelemetry/generateTraces.ts Added startTime parameter to startNestedSpan function.
app/client/src/UITelemetry/generateWebWorkerTraces.ts Added functions and interface for managing WebworkerSpan objects.
app/client/src/sagas/... Instrumented functions with telemetry spans using startRootSpan and endSpan.
app/client/src/utils/WorkerUtil.ts Added conditional logic to convert web worker spans to regular spans.
app/client/src/workers/Evaluation/handlers/evalTree.ts Added profiling logic using startSpansInAnEvaluation.
app/client/src/workers/Evaluation/types.ts Added webworkerTelemetry property to EvalTreeResponseData interface.

Assessment against linked issues

Objective Addressed Explanation
Add more detailed custom traces for Fe coders (#29153) ✅ The changes include the addition of new telemetry functions and spans, which align with the objective of providing more detailed custom traces for frontend coders.
Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

@github-actions github-actions bot added Performance Issues related to performance Performance infra all issue related to the performance infra Performance Pod All things related to Appsmith performance skip-changelog Adding this label to a PR prevents it from being listed in the changelog labels Dec 4, 2023
@vsvamsi1 vsvamsi1 requested review from rajatagrawal and ohansFavour and removed request for ApekshaBhosale December 4, 2023 08:43
@vsvamsi1
Copy link
Contributor Author

vsvamsi1 commented Dec 4, 2023

/ok-to-test sha=1adf68ce0e9b08a19fd615bd990285fb75456d90

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7084079919.
Workflow: Appsmith External Integration Test Workflow.
Tags: ``.

coderabbitai[bot]
coderabbitai bot reviewed Dec 4, 2023
View reviewed changes
app/client/src/UITelemetry/generateWebWorkerTraces.ts
Comment on lines +21 to +22
startTime: Date.now(),
endTime: Date.now(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialization of endTime should not be done at span creation.

-    endTime: Date.now(),
+    endTime: undefined,
Committable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
startTime: Date.now(),
endTime: Date.now(),
startTime: Date.now(),
endTime: undefined,

app/client/src/UITelemetry/generateWebWorkerTraces.ts Outdated Show resolved Hide resolved
app/client/src/UITelemetry/generateWebWorkerTraces.ts Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7084079919.
Commit: 1adf68ce0e9b08a19fd615bd990285fb75456d90.
Cypress dashboard url: Click here!
It seems like there are some failures 😔. We are not able to recognize it, please check this manually here.

@vsvamsi1
Copy link
Contributor Author

vsvamsi1 commented Dec 4, 2023

/ok-to-test sha=8622bf2fafb55ae74b8796c975b68cdf55830d77

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7084552414.
Workflow: Appsmith External Integration Test Workflow.
Tags: ``.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7084552414.
Commit: 8622bf2fafb55ae74b8796c975b68cdf55830d77.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/Apps/CommunityIssues_Spec.ts

To know the list of identified flaky tests - Refer here

@vsvamsi1 vsvamsi1 self-assigned this Dec 4, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7084552414.
Commit: 8622bf2fafb55ae74b8796c975b68cdf55830d77.
Cypress dashboard url: Click here!
All cypress tests have passed 🎉🎉🎉

@trishaanand trishaanand merged commit 904c727 into release Dec 5, 2023
20 checks passed
@trishaanand trishaanand deleted the release1 branch December 5, 2023 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai coderabbitai left review comments

@rajatagrawal rajatagrawal rajatagrawal approved these changes

@ohansFavour ohansFavour Awaiting requested review from ohansFavour

Assignees

@vsvamsi1 vsvamsi1

Labels
Performance infra all issue related to the performance infra Performance Pod All things related to Appsmith performance Performance Issues related to performance skip-changelog Adding this label to a PR prevents it from being listed in the changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add more detailed custom traces for Fe coders
3 participants
@vsvamsi1 @rajatagrawal @trishaanand