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

Cleanup some duplicate telemetry helpers #9016

Merged
merged 6 commits into from
Feb 9, 2022

Conversation

markfields
Copy link
Member

extractLogSafeErrorProperties

This helper existed in both telemetry-utils and common-utils packages. The copy in common-utils was out of date and unused in this repo, and has now been removed. Tests were moved to telemetry-utils and updated.

extractSafePropertiesFromMessage / extractLogSafeMessageProperties

extractLogSafeMessageProperties was a non-exported copy (almost!) of extractSafePropertiesFromMessage. So I removed it and updated callers to use the shared one. The prop names added by the shared one are all prefixed with message, but this was not the case for the duplicate. I will update #9014 with the changes in property names for the two impacted codepaths once this merges.

@markfields markfields requested review from a team as code owners February 4, 2022 19:39
@github-actions github-actions bot added area: loader Loader related issues breaking change This PR or issue would introduce a breaking change labels Feb 4, 2022
@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: -234 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 380.17 KB 380.17 KB No change
containerRuntime.js 180.95 KB 180.95 KB No change
loader.js 158.97 KB 158.74 KB -234 Bytes
map.js 50.64 KB 50.64 KB No change
matrix.js 145.45 KB 145.45 KB No change
odspDriver.js 193.6 KB 193.6 KB No change
odspPrefetchSnapshot.js 45.41 KB 45.41 KB No change
sharedString.js 166.37 KB 166.37 KB No change
Total Size 1.32 MB 1.32 MB -234 Bytes

Baseline commit: 40bafc9

Generated by 🚫 dangerJS against 95258f3

@markfields markfields merged commit 878ed66 into microsoft:main Feb 9, 2022
@markfields markfields deleted the cleanup branch February 9, 2022 00:11
ghost pushed a commit that referenced this pull request Aug 12, 2022
Doesn't seem to make sense for this single logger to live in common-utils -- moving this will make it easier to find and consolidate related code, and I think it aligns with the similar movement in #9016.  This change would also update all existing imports in our repo in preparation for the eventual removal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: loader Loader related issues breaking change This PR or issue would introduce a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants