chore: Semantic Conventions export individual strings #4298
Conversation
| name: SDK_INFO.NAME, | ||
| language: SDK_INFO.LANGUAGE, | ||
| version: SDK_INFO.VERSION, | ||
| name: SDK_INFO[SEMRESATTRS_TELEMETRY_SDK_NAME], |
There was a problem hiding this comment.
This change is needed because the previous test (trying to use the SDK_INFO.NAME etc) was referencing values that where effectively undefined (this ends up cause the test to always check the "default" values and not the real set values.
| @@ -317,14 +322,17 @@ const assertHasOneLabel = (prefix: string, resource: Resource): void => { | |||
|
|
|||
| assert.ok( | |||
| hasOne, | |||
| 'Resource must have one of the following attributes: ' + | |||
| 'Must have one node Resource(s) starting with [' + | |||
| prefix + | |||
There was a problem hiding this comment.
Added extra context to the test to aid in debugging, as this test was failing after my change -- identified and fixed the issue. But found the additional context was useful -- so left it here.
| Object.entries(SemanticResourceAttributes) | ||
| .reduce((result, [key, value]) => { | ||
| if (key.startsWith(prefix)) { | ||
| result.push(value); | ||
| } | ||
| return result; | ||
| }) | ||
| .join(', ') | ||
| .join(', ') + | ||
| JSON.stringify(Object.keys(SemanticResourceAttributes)) |
There was a problem hiding this comment.
As above additional debugging context when the test was failing that was useful.
| [SemanticResourceAttributes.WEBENGINE_NAME]: 'chromium', | ||
| [SemanticResourceAttributes.WEBENGINE_VERSION]: '99', | ||
| [SemanticResourceAttributes.WEBENGINE_DESCRIPTION]: 'Chromium', | ||
| name: 'chromium', |
There was a problem hiding this comment.
Setting the "Real" (valid) defined properties of the SDK_INFO type.
packages/opentelemetry-semantic-conventions/src/internal/constants.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-semantic-conventions/src/resource/SemanticResourceAttributes.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-semantic-conventions/src/resource/SemanticResourceAttributes.ts
Outdated
Show resolved
Hide resolved
| * to a constant map value will result in all strings being included into your bundle. | ||
| * @deprecated Use the SEMRESATTRS_XXXXX constants rather than the SemanticResourceAttributes.XXXXX for bundle minification. | ||
| */ | ||
| export type SemanticResourceAttributes = { |
There was a problem hiding this comment.
Creating a strong type definition of the existing class to make sure that everything is correctly defined and passed, without this strong type it would actually be possible to pass the wrong (or missing) strings to the namespace object construction on line 1164.
Now if you add or remove something that is expected TypeScript is very unhappy.
| * Create exported Value Map for SemanticResourceAttributes values | ||
| * @deprecated Use the SEMRESATTRS_XXXXX constants rather than the SemanticResourceAttributes.XXXXX for bundle minification | ||
| */ | ||
| export const SemanticResourceAttributes: SemanticResourceAttributes = |
There was a problem hiding this comment.
This doesn't need to redefine the documentation as it's referencing the interface which will provide it for this object.
| * @deprecated Use the SEMRESATTRS_XXXXX constants rather than the SemanticResourceAttributes.XXXXX for bundle minification | ||
| */ | ||
| export const SemanticResourceAttributes: SemanticResourceAttributes = | ||
| createConstMap<SemanticResourceAttributes>([ |
There was a problem hiding this comment.
This "helper" effective creates the original object (enforced by the type above) with the converted name and the value.
So by passing the "value" defined by the variable SEMRESATTR_CLOUD_PROVIDER, this is effectively expanded to
CLOUD_PROVIDER: 'cloud.provider'without requiring both the "fixed" (unminificable) property name CLOUD_PROVIDER or the string value (except for the previous declaration of said variable holding the fixed string value of course), so this alone reduces the total "size" of this definition.
packages/opentelemetry-semantic-conventions/src/resource/SemanticResourceAttributes.ts
Outdated
Show resolved
Hide resolved
| * The Amazon Resource Name (ARN) of an [ECS container instance](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ECS_instances.html). | ||
| */ | ||
| export const SEMRESATTRS_AWS_ECS_CONTAINER_ARN = (AWS + | ||
| DOT + |
There was a problem hiding this comment.
While the usage of the DOT and later UNDERSCORE variables seem like a waste because at the very least they cause all "usages" to expand from 1 character . to at least 2 (when minified) -- not counting the declaration of the variable in the first place.
This does allow "common" repeated strings to be reused more than once just because there is a leading / trailing . or _ which taken in totality reduces the overall size of the static strings.
There was a problem hiding this comment.
This is also rationalized across both the SemanticAttributes.ts and SemanticResourceAttributes.ts, the "strings" where also chosen by hand and not automatically parsed to identify the longest common variants, which would have required some post-processing of the jinga template conversion or a LOT more work.
| @@ -17,7 +17,12 @@ | |||
| import { SDK_INFO } from '@opentelemetry/core'; | |||
| import * as assert from 'assert'; | |||
| import { IResource } from '../../src/IResource'; | |||
| import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; | |||
| import { | |||
| SEMRESATTRS_TELEMETRY_SDK_LANGUAGE, | |||
There was a problem hiding this comment.
As the previous names where wrong, just took this opportunity to start using the new exported values rather than re-using the namespaced version.
| @@ -28,7 +32,8 @@ docker run --rm \ | |||
| code \ | |||
| --template /templates/SemanticAttributes.ts.j2 \ | |||
| --output /output/SemanticAttributes.ts \ | |||
| -Dclass=SemanticAttributes | |||
| -Dclass=SemanticAttributes \ | |||
| -Dcls_prefix=SEMATTRS | |||
There was a problem hiding this comment.
Passing the "prefix" for the constants here but not the individual "constant" strings as (at least on windows with git bash shell) the command line gets exceeded and the tool (via docker) fails to run 😢
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4298 +/- ##
==========================================
+ Coverage 92.39% 92.40% +0.01%
==========================================
Files 330 330
Lines 9531 9531
Branches 2036 2036
==========================================
+ Hits 8806 8807 +1
+ Misses 725 724 -1 |
packages/opentelemetry-semantic-conventions/src/internal/constants.ts
Outdated
Show resolved
Hide resolved
MSNev
force-pushed
the
MSNev/SemConv
branch
2 times, most recently
from
5eeb11b
to
fd53f1b
Compare
|
Output from the included size-limit tests using the full strings, while calculating the imports via the The named groups are defined in the Simplistic description of the names
If you run the
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
BundlePhobia: @opentelemetry/semantic-conventions v1.21.0 ❘ Bundlephobia (12.9kb minified), so this is the original size with just the namespaces so this is (probably) equivalent to the eg original includes SemanticConventions {
EXPORTED_NAME1: "exported.name1"
EXPORTED_NAME2: "exported.name2"
EXPORTED_NAME3: "exported.name3"
}where now the name (key) is derived at runtime and so the bundle only lists the value. |
There was a problem hiding this comment.
I guess we could use autoImports.ts to print results somewhere and keep track of the gain/loss when updating further the semantic conventions.
The changes on other packages are quite straight forward which is good but I'd suggest that next step should be to think on how to update the specs in a way that enables migrations like the one explained here https://opentelemetry.io/blog/2023/http-conventions-declared-stable/
packages/opentelemetry-semantic-conventions/src/resource/SemanticResourceAttributes.ts
Outdated
Show resolved
Hide resolved
|
Updated sizes after adding using
|
There was a problem hiding this comment.
Looks good % changelog nit
Thanks for working on this and thanks for addressing the feedback after the first attempt 🙂
In the future we could consider adding the bundle size test results to the benchmark data at https://opentelemetry.io/docs/languages/js/benchmarks/ as it might be good to have that tracked over time.
|
@open-telemetry/javascript-approvers last call for comments. Planning to merge this tomorrow (Feb 23, 13:00 CET) |
## Which problem is this PR solving? While doing some bundle size analysis, noticed that the `@opentelemetry/semantic-conventions` library is a significant contributor to the bundle size because it has a lot of unminified constants. There was a [PR merged that also exports each constant by itself](open-telemetry/opentelemetry-js#4298) instead of on an object which allows tree shaking to only import that constant instead of a whole object. This PR uses the new constants from the `@opentelemetry/semantic-conventions` instead of the deprecated `SemanticConventions` object. ## Short description of the changes - Use exported constant for service name. ## How to verify that this has the expected result - Smoke tests pass - Service name is still respected, run the example app locally and check that service name is set to the value expected.
…#4298) * chore: Semantic Conventions export individual strings * Reduce to just emit full strings and add size-limit test output to review the results * Update generation to use createConstMap for enums where possible * Move changelog back to Unreleased -- merge shifted it --------- Co-authored-by: Marc Pichler <[email protected]>
Which problem is this PR solving?
This PR emits individual strings independently for the SemanticAttributes and SemanticResourceAttributes along with the existing namespaced object (for backward compatibility). It also "constructs" the static strings at runtime rather than including all of them, to aid in minification of the bundle .
While this initial PR is just introducing these individual strings (and provides better type checking of the values), because all of the existing packages are continuing to use the namespaces it is not expected that this PR will provide a hug minification advantage, it (may) even initial cause the total bundle size to increase (because of the additional exported names).
This is the initial portion of #4185, further effort is needed to complete that issue.
Short description of the changes
Type of change
How Has This Been Tested?
Tested and validated by manually reviewing the generated output (looking for repeated strings and added as needed) and ran all tests locally (on Windows)
Checklist: