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

feat(toolkit-lib): emit marker messages during actions #219

Merged
merged 5 commits into from
Mar 11, 2025

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Mar 10, 2025

Introduces the new concept of "Message Spans" to connect multiple messages for toolkit sub-tasks like "synth" or "build-assets" together. A span consists of a number of messages that have the same span value.

At a minimum, a span is a pair of a start and an end message. Every end message also reports the elapsed time since the start of the span.

Message spans will allow integrators to more easily track the flow of "tasks" completed in the Toolkit.

Changes

  • Refactors the previous Timer class to "Message Spans"
  • Introduces a new registry for spans at the bottom of the existing messages registry. A span consists of a start and an end message maker.
  • This PR makes use of the new IoHelper to provide a nicer API
  • There is some more of type magic to ensure correct payloads, I don't think this can be tested further
  • I also changed the message registry to be private again, we are not ready to have it as part of the public interface (this doesn't affect our packages)
  • Made some improvements to the bundling of declaration files. It's currently really slow because it ends up scanning through A LOT of SDK types. Will look into this more in a follow-up PR.

Fixes aws/aws-cdk#33286


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

@aws-cdk-automation aws-cdk-automation requested a review from a team March 10, 2025 17:56
@github-actions github-actions bot added the p2 label Mar 10, 2025
@mrgrain mrgrain force-pushed the mrgrain/feat/markers-for-actions branch from 7af9478 to 444fefb Compare March 10, 2025 18:19
@mrgrain mrgrain force-pushed the mrgrain/feat/markers-for-actions branch from 444fefb to bc6483c Compare March 10, 2025 18:53
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All added messages are traces and won't show up in regular logging configurations (like our cli).

@mrgrain mrgrain added the pr/exempt-integ-test Skips the integ test steps if set. label Mar 11, 2025
@mrgrain mrgrain force-pushed the mrgrain/feat/markers-for-actions branch from bc6483c to eec20b7 Compare March 11, 2025 11:34
@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.86%. Comparing base (0c21495) to head (d14dbcb).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
+ Coverage   84.84%   84.86%   +0.01%     
==========================================
  Files         208      208              
  Lines       35564    35563       -1     
  Branches     4612     4612              
==========================================
+ Hits        30175    30179       +4     
+ Misses       5239     5237       -2     
+ Partials      150      147       -3     
Flag Coverage Δ
suite.unit 84.86% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mrgrain mrgrain force-pushed the mrgrain/feat/markers-for-actions branch 2 times, most recently from d8266f5 to 3345ac7 Compare March 11, 2025 12:52
/**
* Create a new marker from a given registry entry
*/
public marker<S extends MarkerStart, E extends MarkerEnd>(type: MarkerDefinition<S, E>): Marker<S, E> {
Copy link
Contributor

Choose a reason for hiding this comment

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

...and I would call this something like begin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the ioHelper.span(...).begin(...) approach. API looks nicer to me that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bundling currently takes a while because the declaration bundling started to scan through sdk packages. I plan to fix/work-around/whatever, but in the meantime make this slightly faster by doing all work in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to move to shared because it is used in payloads.

@mrgrain mrgrain force-pushed the mrgrain/feat/markers-for-actions branch from 3345ac7 to 26186ee Compare March 11, 2025 15:20
@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Mar 11, 2025
Merged via the queue into main with commit 84d0b54 Mar 11, 2025
14 checks passed
@aws-cdk-automation aws-cdk-automation deleted the mrgrain/feat/markers-for-actions branch March 11, 2025 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 pr/exempt-integ-test Skips the integ test steps if set.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit markers
4 participants