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

[V3] Legacy metrics migration #2086

Merged
merged 1 commit into from
Mar 9, 2022
Merged

[V3] Legacy metrics migration #2086

merged 1 commit into from
Mar 9, 2022

Conversation

xuesichao
Copy link
Contributor

@xuesichao xuesichao commented Mar 5, 2022

Issue #1320 :

Description of changes:
Update the legacy getStats metrics to standardized metrics.

Testing:

Can these tested using a demo application? Please provide reproducible step-by-step instructions.

Yes, browser demo. Tested in browser demo in Chrome, Firefox and Safari and confirmed the change works as expected.

  • Install JS SDK with this change in browser demo
  • Run meeting demo with npm run start:hot to enable debugging
  • Add breakpoint here:
    this.sendClientMetricProtobuf(clientMetricFrame);
  • Open demo demo in Chrome/Firefox/Safari
  • Debug the demo and check if the metrics are collected correctly:
    • clientMetricFrame
    • filteredRawMetricReports
    • getObservableMetrics()
    • getObservableVideoMetrics()
    • getRTCStatsReport()

Checklist:

  1. Have you successfully run npm run build:release locally?
    Yes

  2. Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
    Yes

  3. Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
    Yes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@xuesichao xuesichao requested a review from a team as a code owner March 5, 2022 02:26
@xuesichao
Copy link
Contributor Author

The changes are messy and trivial. Suggest reviewing the changes commit by commit. Will squash the commits and rebase in the end.

demos/browser/app/meetingV2/meetingV2.ts Show resolved Hide resolved
src/clientmetricreport/ClientMetricReport.ts Show resolved Hide resolved
src/clientmetricreport/ClientMetricReport.ts Show resolved Hide resolved
src/statscollector/DefaultStatsCollector.ts Outdated Show resolved Hide resolved
src/statscollector/DefaultStatsCollector.ts Outdated Show resolved Hide resolved
src/statscollector/DefaultStatsCollector.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ltrung
Copy link
Contributor

ltrung commented Mar 7, 2022

Can you put in the title V3?

@xuesichao xuesichao changed the title Legacy metrics migration [V3] Legacy metrics migration Mar 7, 2022
vidya-mohan
vidya-mohan previously approved these changes Mar 8, 2022
Copy link
Contributor

@vidya-mohan vidya-mohan left a comment

Choose a reason for hiding this comment

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

Nice job! Not blocking- you may wish to quickly review existing guides that reference these for eg availableSendBandwidth etc and modify to the new one

@xuesichao
Copy link
Contributor Author

Nice job! Not blocking- you may wish to quickly review existing guides that reference these for eg availableSendBandwidth etc and modify to the new one

@vidya-mohan FAQ is the only doc that mentions the metric specs, updated the FAQ to remove the legacy specs.

@xuesichao
Copy link
Contributor Author

My concern was that migration doc is used once when going from 2->3. API docs are consumed everytime after. Its okay to soft link it to the documentation in migration docs but wonder if IDEs will auto-populate and it would be natural for developers to check on the API documentation when they are actively coding

I agree with you that it is very helpful to mention that. These are interface definitions which should not be that specific in my opinion. The current statement is clear enough.

I would prefer writing a guide on these metrics later.

Copy link
Contributor

@hensmi-amazon hensmi-amazon left a comment

Choose a reason for hiding this comment

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

lgtm!

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

Successfully merging this pull request may close these issues.

5 participants