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

[Telemetry API] Prevent Subscriptions with different options from overwriting each other #7930

Merged
merged 14 commits into from
Dec 4, 2024

Conversation

jvigliotta
Copy link
Contributor

@jvigliotta jvigliotta commented Nov 14, 2024

Closes #7933

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?

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.53%. Comparing base (ba4d8a4) to head (1922b67).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7930      +/-   ##
==========================================
+ Coverage   57.43%   57.53%   +0.09%     
==========================================
  Files         677      677              
  Lines       27375    27404      +29     
  Branches     2690     2690              
==========================================
+ Hits        15724    15766      +42     
+ Misses      11310    11297      -13     
  Partials      341      341              
Flag Coverage Δ
e2e-ci 62.69% <100.00%> (+0.21%) ⬆️
e2e-full 41.91% <66.66%> (+0.04%) ⬆️
unit 49.33% <66.66%> (+0.02%) ⬆️
Files with missing lines Coverage Δ
example/generator/GeneratorMetadataProvider.js 100.00% <ø> (ø)
example/generator/StateGeneratorProvider.js 100.00% <100.00%> (ø)
src/api/telemetry/TelemetryAPI.js 90.03% <100.00%> (+0.82%) ⬆️

... and 6 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 ba4d8a4...1922b67. Read the comment docs.

@jvigliotta jvigliotta requested a review from akhenry November 20, 2024 20:01
@jvigliotta jvigliotta marked this pull request as ready for review November 20, 2024 20:03
Copy link
Contributor

@akhenry akhenry left a comment

Choose a reason for hiding this comment

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

I think the hash code is the right approach here, thanks. I think we can simplify the code a little with a few tweaks,

src/api/telemetry/TelemetryAPI.js Outdated Show resolved Hide resolved
src/api/telemetry/TelemetryAPI.js Outdated Show resolved Hide resolved
src/api/telemetry/TelemetryAPI.js Fixed Show resolved Hide resolved
@jvigliotta jvigliotta requested a review from akhenry November 26, 2024 22:18
src/api/telemetry/TelemetryAPI.js Outdated Show resolved Hide resolved
src/api/telemetry/TelemetryAPI.js Outdated Show resolved Hide resolved
// Wait for some data to populate
// eslint-disable-next-line playwright/no-wait-for-timeout
await page.waitForTimeout(2000); // Wait for a few state changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well then... glad I did ALL THAT. Haha, let me switch this out and test to make sure.

Copy link
Contributor Author

@jvigliotta jvigliotta Dec 3, 2024

Choose a reason for hiding this comment

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

Actually, going back, I remember why I did this. The table is adding and removing rows basically at the same time, so I can't go by count, because the rows aren't being added. So what I'm doing here is observing added nodes to the table child list (tr's would count). I wait for 20 rows to be added, then I verify there are non-filtered rows shown and filtered rows NOT shown. Not the simplest way to go about it, but I'm open to options.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand, you want to count 20 add events, not 20 total rows.

@akhenry akhenry added this to the Target:4.1.0 milestone Dec 3, 2024
@akhenry akhenry enabled auto-merge (squash) December 3, 2024 23:12
@akhenry akhenry added the pr:e2e:couchdb npm run test:e2e:couchdb label Dec 3, 2024
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Dec 3, 2024
@akhenry akhenry added the pr:e2e:couchdb npm run test:e2e:couchdb label Dec 4, 2024
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Dec 4, 2024
@akhenry akhenry merged commit 61b982a into master Dec 4, 2024
23 checks passed
@akhenry akhenry deleted the viperomct-552-553 branch December 4, 2024 03:33
akhenry pushed a commit that referenced this pull request Dec 4, 2024
…rwriting each other (#7930)

* initial implementation

* cleaning up a bit

* adding the hash method back as we dont want gigantic keys

* adding a line

* added filtering to state generator, updated filters readme to fix error, more robust hash function

* removing unnecessary changes in wrong file

* adding a test to confirm each endpoint has a separate subscription based of filtering

* lint

* adding back in hints, accidentally removed

* remove some redundant code and convert sanitization method into a replacer function for stringify

* tweaking serialize replacer to handle arrays correctly, adding more determinative row addition check to test

* more focused selector for the table

* simplified the serialization method even further and added some more docs
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.

[TelemetryAPI] Subscriptions need to be unique based on request options
2 participants