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

chore: update react package telemetry config #17837

Merged

Conversation

mattrosno
Copy link
Member

@mattrosno mattrosno commented Oct 22, 2024

A while back, the FeatureFlags component was updated from a single prop of type object, to one prop per feature, so this data can be captured by IBM Telemetry. These new props need added to the telemetry.yml's allowedAttributeNames.

I ran this command in the /packages/react directory:

npx -y @ibm/telemetry-js-config-generator generate --id fd89d12b-6a39-48b4-adf4-30c94dc0dddd --endpoint https://www-api.ibm.com/ibm-telemetry/v1/metrics --files ./src/components/**/*.(tsx|js|jsx)

The Telemetry team maintains @ibm/telemetry-js-config-generator, although it's up to you how you use it. E.g. manually if you add a net-new prop or prop value, or pro mode would be automatically somehow.

Changelog

New

  • N/A

Changed

  • telemetry.yml re-generated

Removed

  • N/A

Testing / Reviewing

I can't figure out why the new YML files doesn't have quotes anymore, making the PR more difficult to diff. This view seems best for this PR:

image

You should see new props included if no components used them the last time this file was generated. For example, enableV12Overflowmenu.

Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit e0aaa81
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/671aa825501125000838fa4f
😎 Deploy Preview https://deploy-preview-17837--v11-carbon-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit e0aaa81
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/671aa8251e04e0000884d156
😎 Deploy Preview https://deploy-preview-17837--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit e0aaa81
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/671aa825501125000838fa4d
😎 Deploy Preview https://deploy-preview-17837--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mattrosno mattrosno marked this pull request as ready for review October 22, 2024 21:38
@mattrosno mattrosno requested a review from a team as a code owner October 22, 2024 21:38
@tay1orjones tay1orjones requested a review from a team as a code owner October 23, 2024 13:59
@tay1orjones tay1orjones requested a review from kennylam October 23, 2024 13:59
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Thanks for this! Instead of running it automatically I added a commit that updates ci to check if the file needs updated. It's basically the same idea as the format check where it fails if changes are needed - you have to make the change locally and push up the changes to get the status check to pass.

Let me know what you think - I can revert that commit if there's some gaps I missed.

@mattrosno
Copy link
Member Author

@tay1orjones I like it. I didn't make any changes to the telemetry.yml after the config generator did its thing, so post merge, the check should continue working.

@mattrosno
Copy link
Member Author

@tay1orjones I guess this could be used in the 3 other React-based packages where each script command uses its own project ID: https://github.com/search?q=repo%3Acarbon-design-system%2Fcarbon%20allowedAttributeNames&type=code

But if these other packages rarely add/remove component props, it might not be worth the effort.

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.19%. Comparing base (3141442) to head (e0aaa81).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #17837   +/-   ##
=======================================
  Coverage   80.19%   80.19%           
=======================================
  Files         406      406           
  Lines       14040    14040           
  Branches     4347     4395   +48     
=======================================
  Hits        11260    11260           
+ Misses       2614     2613    -1     
- Partials      166      167    +1     

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

@tay1orjones
Copy link
Member

@mattrosno great point - I'll push an update and fix the new check too, it needs a yarn install

@tay1orjones tay1orjones self-assigned this Oct 24, 2024
@tay1orjones
Copy link
Member

I couldn't figure out why the CI check was reporting a difference in the config - whenever I would run it locally it was fine. I tried a bunch of stuff but gave up. Left in the script though so it's easier to update this!

@tay1orjones tay1orjones enabled auto-merge October 24, 2024 20:05
@tay1orjones tay1orjones added this pull request to the merge queue Oct 24, 2024
Merged via the queue into carbon-design-system:main with commit 106e804 Oct 24, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants