Skip to content

Conversation

@akhenry
Copy link
Contributor

@akhenry akhenry commented Sep 11, 2024

Closes #7838

Describe your changes:

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this a notable change that will require a special callout in the release notes? For example, will this break compatibility with existing APIs or projects that consume these plugins?

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Has this been smoke tested?
  • Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
  • Have you associated a milestone with this PR? Note: leave blank if unsure.
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?

@akhenry akhenry marked this pull request as draft September 11, 2024 03:39
@codecov
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 35.95506% with 57 lines in your changes missing coverage. Please review.

Project coverage is 57.00%. Comparing base (c498f7d) to head (c69b6ab).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/api/telemetry/WebSocketWorker.js 0.00% 30 Missing ⚠️
src/api/telemetry/BatchingWebSocket.js 10.00% 27 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7837      +/-   ##
==========================================
- Coverage   57.25%   57.00%   -0.25%     
==========================================
  Files         675      675              
  Lines       27287    27317      +30     
  Branches     2671     2671              
==========================================
- Hits        15622    15572      -50     
- Misses      11329    11409      +80     
  Partials      336      336              
Flag Coverage Δ
e2e-ci 62.01% <35.95%> (+0.21%) ⬆️
e2e-full 23.47% <4.49%> (-18.49%) ⬇️
unit 49.32% <10.11%> (-0.04%) ⬇️
Files with missing lines Coverage Δ
src/plugins/performanceIndicator/plugin.js 94.11% <100.00%> (+7.16%) ⬆️
src/api/telemetry/BatchingWebSocket.js 4.00% <10.00%> (-0.62%) ⬇️
src/api/telemetry/WebSocketWorker.js 0.00% <0.00%> (ø)

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c498f7d...c69b6ab. Read the comment docs.

@akhenry akhenry marked this pull request as ready for review September 11, 2024 18:58
@akhenry akhenry added this to the Target:4.1.0 milestone Sep 11, 2024
@github-actions github-actions bot removed the pr:e2e:perf Trigger perf tests label Sep 11, 2024
Copy link
Contributor

@unlikelyzero unlikelyzero left a comment

Choose a reason for hiding this comment

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

  • I would consider moving the the addEventListener('DOMContentLoaded' approach
  • Don't declare the const locators if the variable is only read once
  • Consider a regular label instead of aria or use arialabeledby

Copy link
Contributor

@shefalijoshi shefalijoshi left a comment

Choose a reason for hiding this comment

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

Small changes. Looks good otherwise.

@shefalijoshi shefalijoshi self-requested a review September 27, 2024 18:53
Copy link
Contributor

@shefalijoshi shefalijoshi left a comment

Choose a reason for hiding this comment

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

Nice catch! Yay for tests!

@akhenry akhenry added the pr:e2e:couchdb npm run test:e2e:couchdb label Sep 30, 2024
@akhenry akhenry enabled auto-merge (squash) September 30, 2024 19:57
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Sep 30, 2024
@akhenry akhenry added pr:e2e:couchdb npm run test:e2e:couchdb notable_change A change which should be noted in the changelog labels Sep 30, 2024
@akhenry akhenry removed the request for review from unlikelyzero September 30, 2024 21:08
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Sep 30, 2024
@akhenry akhenry disabled auto-merge September 30, 2024 21:33
@akhenry akhenry merged commit 29f1956 into master Sep 30, 2024
@akhenry akhenry deleted the improved-buffering branch September 30, 2024 21:36
@akhenry akhenry modified the milestones: Target:4.1.0, Build 9 RC11 Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notable_change A change which should be noted in the changelog type:enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve performance and usability of telemetry batching implementation

5 participants