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

Populate package:http_profile #1046

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Populate package:http_profile #1046

merged 8 commits into from
Feb 21, 2024

Conversation

derekxu16
Copy link
Member

No description provided.

brianquinlan
brianquinlan reviewed Nov 14, 2023
View reviewed changes
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
bkonyi
bkonyi reviewed Nov 15, 2023
View reviewed changes
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
@derekxu16 derekxu16 requested review from aam and a-siva November 16, 2023 19:06
@derekxu16 derekxu16 force-pushed the populate-package-http-profile branch 2 times, most recently from 3001781 to 206a7ff Compare December 4, 2023 16:40
brianquinlan
brianquinlan reviewed Dec 4, 2023
View reviewed changes
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
bkonyi
bkonyi reviewed Dec 4, 2023
View reviewed changes
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
pkgs/http_profile/lib/http_profile.dart Outdated Show resolved Hide resolved
@derekxu16 derekxu16 removed request for aam and a-siva December 4, 2023 19:13
@derekxu16 derekxu16 force-pushed the populate-package-http-profile branch 2 times, most recently from b667918 to 6618444 Compare December 4, 2023 22:21
brianquinlan
brianquinlan approved these changes Dec 7, 2023
View reviewed changes
Copy link
Collaborator

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

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

I'm OK with submitting this. Before any sort of release you will need tests and some way to demonstrate that it actually works.

Do you have an SDK patch that works with this?

I'd be happy to try this out.

@derekxu16
Copy link
Member Author

I still need to update the types in the service extension spec to make them compatible with the types here. I've made those changes locally and checked the ways in which DevTools needs to be updated in response to them. I've noticed that it would be best if DevTools didn't need to consider when requestStartTimestamp, requestMethod or requestUri are null. Can I make these required parameters of HttpClientRequestProfile.profile?

@brianquinlan
Copy link
Collaborator

I still need to update the types in the service extension spec to make them compatible with the types here. I've made those changes locally and checked the ways in which DevTools needs to be updated in response to them. I've noticed that it would be best if DevTools didn't need to consider when requestStartTimestamp, requestMethod or requestUri are null. Can I make these required parameters of HttpClientRequestProfile.profile?

I think so. I don't think that that an HTTP request would be meaningful without those.

@derekxu16
Copy link
Member Author

Cool, sounds good. And then regarding the comment above about needing tests, did you mean unit tests? I can add unit tests now. I also want to add integration tests ensuring that the service extensions correctly return profiling information recorded using this package, but I won't be able to add those until this package is published.

@brianquinlan
Copy link
Collaborator

Cool, sounds good. And then regarding the comment above about needing tests, did you mean unit tests?

Yep.

I can add unit tests now. I also want to add integration tests ensuring that the service
extensions correctly return profiling information recorded using this package, but
I won't be able to add those until this package is published.

Why not?

@derekxu16
Copy link
Member Author

Why not?

We would need to have a VM Service to communicate with in the integration tests. I guess we could copy this helper into this package, and then use that to write the tests. I was thinking of just adding them topackage:vm_service's tests though.

@brianquinlan
Copy link
Collaborator

Let me know if I am blocking you from landing this.

@derekxu16
Copy link
Member Author

It can't be landed yet because I'm still in the process of rolling the new package:vm_service version into DevTools.

@derekxu16 derekxu16 marked this pull request as ready for review January 8, 2024 19:14
@brianquinlan
Copy link
Collaborator

Do the tests/lint get run automatically? I would guess not because you don't have a github workflow setup to do that. If, when you sync this PR to main, there are no presubmit failures, you can just submit and I'll setup the automation.

@derekxu16 derekxu16 force-pushed the populate-package-http-profile branch 2 times, most recently from 276f28d to 25f47ea Compare February 15, 2024 21:36
@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Feb 15, 2024
@derekxu16 derekxu16 force-pushed the populate-package-http-profile branch 2 times, most recently from 5cd3c6e to c7e18ce Compare February 16, 2024 14:36
@derekxu16 derekxu16 force-pushed the populate-package-http-profile branch from c7e18ce to fbd08b8 Compare February 16, 2024 14:47
@derekxu16 derekxu16 force-pushed the populate-package-http-profile branch from d7f7868 to 653e5da Compare February 21, 2024 15:23
@derekxu16 derekxu16 merged commit ce0de37 into dart-lang:master Feb 21, 2024
25 checks passed
@derekxu16 derekxu16 deleted the populate-package-http-profile branch February 21, 2024 15:30
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 21, 2024
@devoncarew
[deps] rev async, browser_launcher, dartdoc, http, markdown, protobuf…
7c223d8 
…, sse, stream_channel, tools, vector_math, web, webdev, yaml_edit

Revisions updated by `dart tools/rev_sdk_deps.dart`.

async (https://github.com/dart-lang/async/compare/24266ca..6cdbc41):
  6cdbc41  2024-02-15  Kevin Moore  Update to latest lints, require Dart 3.2 (dart-archive/async#267)

browser_launcher (https://github.com/dart-lang/browser_launcher/compare/74a0efe..7956230):
  7956230  2024-02-16  sigmundch  Add extra flags to disable throttling behavior. (dart-archive/browser_launcher#55)

dartdoc (https://github.com/dart-lang/dartdoc/compare/7e171fc..7a9df65):
  7a9df65f  2024-02-20  Parker Lougheed  Add fallback text for sidebar failing to load (dart-lang/dartdoc#3643)
  9bcabb50  2024-02-20  Parker Lougheed  Fix missing left sidebar on extension type pages (dart-lang/dartdoc#3662)
  e8b8faa2  2024-02-16  Sam Rawlins  Include extension types in 'implementers' list (dart-lang/dartdoc#3658)

http (https://github.com/dart-lang/http/compare/6d9f9ef..ce0de37):
  ce0de37  2024-02-21  Derek Xu  Populate package:http_profile (dart-lang/http#1046)
  75e01f4  2024-02-20  Brian Quinlan  Create a simple WebSocket interface (dart-lang/http#1128)

markdown (https://github.com/dart-lang/markdown/compare/c2b8429..d735b0b):
  d735b0b  2024-02-21  Tom Yeh  Fix `#578`: list with checkbox mixed with empty lines (dart-lang/markdown#583)
  6efe141  2024-02-14  Kevin Moore  Migrate example to pkg:web, update minimum required Dart version (dart-lang/markdown#582)

protobuf (https://github.com/dart-lang/protobuf/compare/a293fb9..f085bfd):
  f085bfd  2024-02-20  Ömer Sinan Ağacan  Fix message_set.dart copyright year (google/protobuf.dart#912)

sse (https://github.com/dart-lang/sse/compare/af7d8d0..13ec752):
  13ec752  2024-02-20  Kevin Moore  blast_repo fixes (dart-lang/sse#104)
  2830dc9  2024-02-16  Kevin Moore  Support the latest pkg:web, require Dart 3.3 (dart-lang/sse#103)

stream_channel (https://github.com/dart-lang/stream_channel/compare/851336f..e02a5dd):
  e02a5dd  2024-02-16  Kevin Moore  Require Dart 3.3, update and fix lints (dart-lang/stream_channel#100)
  e62706e  2024-02-16  Kevin Moore  blast_repo fixes (dart-lang/stream_channel#101)

tools (https://github.com/dart-lang/tools/compare/2ef7673..9f4e6a4):
  9f4e6a4  2024-02-16  Elias Yishak  Helper to resolve dart version for clients of analytics (dart-lang/tools#233)
  8323b21  2024-02-13  Elias Yishak  New event added for sending analytics within package on errors (dart-lang/tools#229)

vector_math (https://github.com/google/vector_math.dart/compare/cb976c7..3706feb):
  3706feb  2024-02-18  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2 (google/vector_math.dart#313)

web (https://github.com/dart-lang/web/compare/a54a1f0..975e55c):
  975e55c  2024-02-15  Kevin Moore  Add TrustedTypes (dart-lang/web#173)
  0447807  2024-02-15  Srujan Gaddam  Add info on generation conventions (dart-lang/web#171)

webdev (https://github.com/dart-lang/webdev/compare/629c632..51b5484):
  51b54843  2024-02-14  Elliott Brooks  Implement `setFlag` for 'pause_isolates_on_start' (dart-lang/webdev#2373)

yaml_edit (https://github.com/dart-lang/yaml_edit/compare/2a9a11b..82ab64d):
  82ab64d  2024-02-21  Danny Tuppeny  Fix line endings for inserted maps on Windows (dart-lang/yaml_edit#66)
  6906ac4  2024-02-20  Devon Carew  update the publish workflow (dart-lang/yaml_edit#67)

Change-Id: I246c393586e3d6239925ac3cf3a6a245d86a2bf5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/353581
Reviewed-by: Kevin Moore <[email protected]>
Commit-Queue: Devon Carew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkonyi bkonyi bkonyi left review comments

@brianquinlan brianquinlan brianquinlan approved these changes

Assignees
No one assigned
Labels
type-infra A repository infrastructure change or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@derekxu16 @brianquinlan @bkonyi