-
Notifications
You must be signed in to change notification settings - Fork 542
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: add instrumentation metadata to package.json #2292
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2292 +/- ##
==========================================
- Coverage 90.97% 90.30% -0.68%
==========================================
Files 146 147 +1
Lines 7492 7263 -229
Branches 1502 1509 +7
==========================================
- Hits 6816 6559 -257
- Misses 676 704 +28 |
@open-telemetry/javascript-approvers would love to get feedback on this approach as an MVP + demonstration of how it can be actually leveraged by a tool (lint readme). Please feel free to share any concerns or support for documenting the opentelemetry-domain-specific metadata for the components each npm package implements. I am motivated to work on the followup metadata properties and utilize them, and would be happy to have this one in to build the next steps on top of it |
more on the motivation and future plans can be found in open-telemetry/opentelemetry-js#4725. any feedback is appreciated 🙏 |
"opentelemetry": { | ||
"components": [ | ||
{ | ||
"name": "KafkajsInstrumentation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant on this change.
- It adds a bit of maintenance. For example, this
KafkajsInstrumentation
is out of sync with the code which has a different capitalizationKafkaJsInstrumentation
. - Given this being broken up into an array of
components
, I understand the desire to have a"name"
field for each component. However, that information isn't being used anywhere, so the maintenance of having something that can get out of sync with the code is unfortunate. Do you have a particular use case for the enumeration of these named components? Perhaps some specifics around the "OpenTelemetry control planes" or "Enhancements for UIs" mentioned in #4725 would help. - The use of the "plaforms" field is (at least currently) only in
scripts/lint-readme.js
. However, that lint usage ofisNode, isWeb
is used for adding a README section about inclusion in auto-instrumentations-node or -web. Isn't the current lint-readme.js querying of auto-instrumentations-node/-web the more accurate information? Recent discussion about some new instrumentations targetting "web" were not going to be included in auto-instrumentations-web, IIRC.
What if we added just this?
"opentelemetry": {
"targetPlatforms": ["node"]
},
Granted that a package could export components, some of which target web and some of which target node, but I wouldn't think it likely.
(Another key that might be useful is "stability" or "stabilityLevel" using the values defined at https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/CONTRIBUTING.md#component-lifecycle)
At the least, it would be helpful to have a doc section somewhere (GUIDELINES.md seems okay, but currently is scoped on being an instrumentation implementation guide) that explains what this "opentelemetry" package.json key is about (stability, schema) when people ask.
This is the first implementation of open-telemetry/opentelemetry-js#4725 into the contrib repo.
It adds metadata on the OpenTelemetry components that are exported from this npm package (which at the moment is one per package, but not always the case, for example
opentelemetry-resource-detector-aws
which exports multiple detectors.For each component, one can currently publish its:
name
the name of the component as exported from the package. currently not used in this PR but intended to be added to auto-create the code example for each instrumentation.componentType
- "instrumentation". In the future we can add metadata to more components and this field can identify if the component is "instrumentation" / "sampler" / "propagator" / resource-detector" etc.platforms
- an array with either "node" or "web". instrumentations are always either web or node, but I want to keep it generic so we can mark future non-instrumentation components as both web and node, or add additional platforms if the need arise (like deno).I have updated the lint-markdown script to use this info directly from the package itself, instead of reading it indirectly from the auto-instrumentation packages dependencies.
After this one merges, I will work on adding and utilizing more metadata as followup PRs as described in the issue.